-
Notifications
You must be signed in to change notification settings - Fork 263
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
Updates spf13/cobra dep to v0.0.5 #443
Updates spf13/cobra dep to v0.0.5 #443
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: navidshaikh 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 |
a0f5657
to
02fc0d5
Compare
|
/hold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
go.mod
Outdated
@@ -29,3 +29,5 @@ require ( | |||
knative.dev/test-infra v0.0.0-20190730202142-17f2331e80ad | |||
sigs.k8s.io/yaml v1.1.0 | |||
) | |||
|
|||
replace github.com/spf13/cobra => github.com/chmouel/cobra v0.0.0-20190820110723-8f09bb39b20d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we track that we remove the temporary replacement when the next cobra release is out ?
otherwise I think its ok for me to include this bug-fix release here (it's about escaping [
in the help message, right ?)
But we should decide first on #439 which direction we want to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't track that, we can open a ticket though in client to switch to spf13/cobra and we could cross check part of triaging client issues.
otherwise I think its ok for me to include this bug-fix release here (it's about escaping [ in the help message, right ?)
Yes.
About the comment on #439
if we don't use [ in our help messages)
There are some flags we inherit for machine readable output like --template
whose help text we don't control. :-/
About using cobra from forked bug fix branch, I'd have deferred it if cobra PR turn around is quicker, apparently the PR from Chmouel is open since a while and this ended up even tkn using the fork of cobra.
@navidshaikh oops, I tried to resolve the conflict in |
43d33dd
to
c6296a8
Compare
/retest |
Cross platform build failing with |
hmm, let me check that later today. |
@navidshaikh Looks like that the revision oc @chmouel fork is not available anymore but you had still a cache version. For me it is
|
Oops sorry about this guys, i'll make sure to update the forks in a backward compatible way and ping you all before doing it, |
and fyi it's a security release, viper 1.4.0 ie (spf13/cobra#978) you may want to mention it in your release changes |
1a57a4b
to
b349109
Compare
Fixes knative#426 - and a couple of patches on top which aren't merged yet - spf13/cobra#884 - spf13/cobra#899 - also updates viper to 1.4.0
b349109
to
9e2ca30
Compare
Builds fine on my local machine, tried removing the cached version of vendored deps and re-populate it as well, but the build tests are failing in CI prompting
It reports that additional line in go.sum is missing from the PR change-set, running |
@navidshaikh I think it also depends on the It's really something that we also should nail down when doing such sort of picky comparisons, like for that dep check. Not sure if go allows for fixing a 'format version' or if such thing just can slip through. @navidshaikh could you check which go version is used on CI and which on your local machine, and if that differs, try locally with the same go version as on prow ? |
Local:
On the prow side, it doesn't print the Z stream version for go though
|
hmm. Then it looks like that something other is different local vs server side. Let me check that, too. |
I did some investigations on the Let me try to fix that in a subsequent PR. @navidshaikh for now, just run Some good
|
as a result of `go install golang.org/x/tools/cmd/goimports`
Thanks @rhuss! I've updated the PR. |
/hold cancel |
/lgtm |
/retest |
* this is to create tools/infra oncall.html which will be accessible to OSS community. * Fix whitespaces * remove static mapping * remove static mapping
Fixes #426