Skip to content

Conversation

@sword-jin
Copy link
Contributor

@sword-jin sword-jin commented May 26, 2021

support run singleton. Not always need a goc server

 goc build -o main --debug . --singleton --agentport=:7866

func registerHandlers() {
{{if .Singleton}}
ln, _, err := listen()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this statement seems to avoid the scenario that in Singleton mode, the host variable is defined, but not used, right?

if yes, i prefer to print extra information to enhance it, like:

        ln, host, err := listen()
	if err != nil {
		log.Fatalf("listen failed, err:%v", err)
	}
        profileAddr := "http://" + host
        log.Debugf("[goc] goc profile address: %s \n", profileAddr)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good practice, i'll make change

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems this comment was not resolved, what's happening?

Copy link
Contributor Author

@sword-jin sword-jin Jun 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log output will impact e2e test that assert gocc output @CarlJi

@CarlJi
Copy link
Collaborator

CarlJi commented May 28, 2021

hi @Rrylee , thanks for your contributions. 👍

Besides the review comments and fix failed ci jobs, it's also great to add a e2e case to cover the singleton scenario from goc CLI side. You can refer this for example https://github.com/qiniu/goc/blob/master/tests/build.bats#L36

@qiniu-bot qiniu-bot added size/L and removed size/XXL labels May 29, 2021
@codecov
Copy link

codecov bot commented May 30, 2021

Codecov Report

Merging #191 (5bfd5e8) into master (63c7438) will increase coverage by 0.47%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   68.00%   68.48%   +0.47%     
==========================================
  Files          34       34              
  Lines        1738     1745       +7     
==========================================
+ Hits         1182     1195      +13     
+ Misses        451      445       -6     
  Partials      105      105              
Flag Coverage Δ
e2e-1.11.x ∅ <ø> (∅)
e2e-1.12.x ∅ <ø> (∅)
e2e-1.13.x ∅ <ø> (∅)
e2e-1.14.x ∅ <ø> (∅)
e2e-1.15.x ∅ <ø> (∅)
e2e-1.16.x ∅ <ø> (∅)
unittest-1.13.x 68.36% <88.23%> (+0.58%) ⬆️
unittest-1.14.x 68.48% <88.23%> (+0.70%) ⬆️
unittest-1.15.x 68.36% <88.23%> (+0.35%) ⬆️
unittest-1.16.x 68.36% <88.23%> (+0.58%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/run.go 4.87% <0.00%> (-0.13%) ⬇️
pkg/cover/instrument.go 19.04% <ø> (ø)
cmd/cover.go 81.25% <90.90%> (+61.25%) ⬆️
cmd/build.go 63.33% <100.00%> (+1.26%) ⬆️
cmd/commonflags.go 93.93% <100.00%> (+0.18%) ⬆️
cmd/install.go 62.06% <100.00%> (+1.35%) ⬆️
pkg/cover/cover.go 69.62% <100.00%> (+0.28%) ⬆️
pkg/build/gomodules.go 93.54% <0.00%> (-6.46%) ⬇️

@CarlJi
Copy link
Collaborator

CarlJi commented Jun 1, 2021

/lgtm
/approve

@qiniu-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CarlJi, rrylee

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@qiniu-bot qiniu-bot merged commit 45aee23 into qiniu:master Jun 1, 2021
@sword-jin
Copy link
Contributor Author

nice, i can build a new private image ignore goc-server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants