Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add go-bindata-assetfs to glide #752

Merged
merged 6 commits into from
Apr 3, 2018
Merged

Conversation

cory-klein
Copy link
Contributor

Resolves Issue #751

@coveralls
Copy link

coveralls commented Mar 22, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 652a006 on cory-klein:master into 2e6323c on jaegertracing:master.

@pavolloffay
Copy link
Member

CI failure is not related to this change. Fix is #753

@cory-klein could you please sign commits with -s when doing git commit?

@cory-klein
Copy link
Contributor Author

@pavolloffay Done

@yurishkuro
Copy link
Member

did you run glide up? We keep glide.lock under source control too, without it glide install will not install bindata, and you're back to the same problem.

@pavolloffay
Copy link
Member

@cory-klein the change looks good, please just run glide up to add it to lock file

@cory-klein
Copy link
Contributor Author

@pavolloffay Done

@pavolloffay
Copy link
Member

it if failing on travis. Could you please also sing the last commit?

@cory-klein
Copy link
Contributor Author

Doh, sorry forgot the -s on that last one. Re-committed with it included this time.

@cory-klein
Copy link
Contributor Author

cory-klein commented Mar 27, 2018

Build is failing here:

$ if [ "$CROSSDOCK" == true ]; then bash ./scripts/travis/build-crossdock.sh ; else echo 'skipping crossdock'; fi
CGO_ENABLED=0 GOOS=linux installsuffix=cgo go build -o ./cmd/agent/agent-linux -ldflags "-X github.com/jaegertracing/jaeger/pkg/version.commitSHA=0decadb5a363030e422660e836fac82c4eaaab6c -X github.com/jaegertracing/jaeger/pkg/version.latestVersion=v1.2.0 -X github.com/jaegertracing/jaeger/pkg/version.date=2018-03-27T19:04:01Z" ./cmd/agent/main.go
# github.com/jaegertracing/jaeger/vendor/github.com/spf13/viper
vendor/github.com/spf13/viper/viper.go:1371:10: cannot assign 1 values to 2 variables
make: *** [build-agent-linux] Error 2
The command "if [ "$CROSSDOCK" == true ]; then bash ./scripts/travis/build-crossdock.sh ; else echo 'skipping crossdock'; fi" exited with 2.

But it's not apparent to me whether this could be caused by changes specific to this PR.

The cited viper.go code is here. Here is an excerpt:

		t, err := toml.TreeFromMap(c)
		if err != nil {
			return ConfigMarshalError{err}
}

@black-adder
Copy link
Contributor

Looks like a breaking change was introduced in viper? Do you mind pinning viper to 25b30aa063fc18e48662b86996252eabdcf2f0c7 in glide.yaml and see if that passes?

Signed-off-by: Cory Klein <cory.klein@saucelabs.com>
Signed-off-by: Cory Klein <cory.klein@saucelabs.com>
Signed-off-by: Cory Klein <cory.klein@saucelabs.com>
Signed-off-by: Cory Klein <cory.klein@saucelabs.com>
Signed-off-by: Cory Klein <cory.klein@saucelabs.com>
@cory-klein
Copy link
Contributor Author

@pavolloffay Mind checking one last time? All automated build checks finally passed.

glide.yaml Outdated
@@ -46,11 +46,12 @@ import:
- package: github.com/spf13/cobra
version: 0.0.1
- package: github.com/spf13/viper
version: ^1
version: 25b30aa063fc18e48662b86996252eabdcf2f0c7
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary, it looks like viper did a bad release 1.0.1 and then rolled back the change and re-released as 1.0.2, so our pin ^1 should still work.

Signed-off-by: Cory Klein <cory.klein@saucelabs.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

thanks!

@cory-klein
Copy link
Contributor Author

@yurishkuro Everything looks green! 😄

@yurishkuro yurishkuro merged commit 51c3bc8 into jaegertracing:master Apr 3, 2018
@yurishkuro
Copy link
Member

great, thanks!

mabn pushed a commit to mabn/jaeger that referenced this pull request Apr 3, 2018
* master:
  Configure Gorilla router to allow slash in path params (jaegertracing#736)
  Add go-bindata-assetfs to glide (jaegertracing#752)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants