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

Discussion point: ability to specify Flags prefix #502

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carldunham
Copy link
Contributor

This is a hacky way to do this, but should work. Open to other options.

@nodo
Copy link
Collaborator

nodo commented Jul 15, 2018

Thanks for you PR @carldunham, I really appreciate that you took the time to fix the docs for contributions and go vet. If you wish, I would be happy to merge those in a separate PR and keep this one for discussing the prefix problem.

Coming back to that, what is the actual problem you would like to solve?

As far as I understand with this PR the compiled test file will have duplicate options with ginkgo and ginkgo0 prefix (in case init() runs only once). I think this duplication is not desirable. Moreover, I would not change the flags prefix (at least by default), as I am fairly sure there are users of ginkgo that use go test directly.

@carldunham
Copy link
Contributor Author

@nodo Happy to split the PR. I'll actually submit that separately.

As for the extant issue, I ran into this basically because I was refusing to bend to Go's will with respect to project structure, mainly due to existing project constraints (multiple languages, other devs, existing conventions, etc). It also seems to be the same problem raised in #234.

I provided a toy project that also illustrates the issue in https://github.com/carldunham/go-dep-test.

It should be something that could be fixed in later versions of Go (1.11 module support, perhaps), but still could arise in cases where different versions of Ginkgo were required by different modules, say.

The suggestion in the PR is foolish, as you gently didn't say 😸, but I wasn't able to do something cleaner in a short time. A more rational approach might just be to avoid setting the flags on subsequent calls to init(), but I was curious about the existence of prefix and wondered what I could do with it.

Thanks!

@carldunham carldunham force-pushed the cd/234/allow-prefix-option branch from 48bd5be to 5a73661 Compare July 17, 2018 16:18
@jianzhangbjz
Copy link
Contributor

Still get the flag redefined: ginkgo.seed error even if use this fixed PR. Do you know how to solve it? Thanks!

mac:extended-platform-tests jianzhang$ ./extended-platform-tests run all --dry-run
./extended-platform-tests flag redefined: ginkgo.seed
panic: ./extended-platform-tests flag redefined: ginkgo.seed

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc000178540, 0x8757400, 0xd0c57a0, 0xc00364a042, 0xb, 0x7c2bd0e, 0x2a)
	/usr/local/Cellar/go/1.13.1/libexec/src/flag/flag.go:848 +0x4ae
flag.(*FlagSet).Int64Var(...)
	/usr/local/Cellar/go/1.13.1/libexec/src/flag/flag.go:673
github.com/openshift/origin/vendor/github.com/onsi/ginkgo/config.Flags(0xc000178540, 0x7b541f6, 0x6, 0x5719001)
	/Users/jianzhang/goproject/src/github.com/openshift/origin/vendor/github.com/onsi/ginkgo/config/config.go:68 +0x114
github.com/openshift/origin/vendor/github.com/onsi/ginkgo.init.0()
	/Users/jianzhang/goproject/src/github.com/openshift/origin/vendor/github.com/onsi/ginkgo/ginkgo_dsl.go:58 +0x10a

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.

3 participants