-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
Update a few dependencies #69976
Update a few dependencies #69976
Conversation
/assign @lavalamp |
/retest |
/assign sttts |
I thought the yaml package got updated recently? Maybe that's still in flight? I'm not convinced by this justification for the other two updates. And why is mgo getting pulled in now? |
#67594 This PR is not updating godeps.json though. And that is probably the reason builds are failing there. I was not aware it is there.
What is the usual justification to get a dependency updated? (frankly, I was not expecting this PR to face any "resistance"...). Is that process documented somewhere? Looks like
It is a dependency of What worries you re. this PR? Potential regressions or something else too? |
fyi @dims |
@ash2k is the list of the packages in the commit message correct? here's what i see in the patch and i don't see 10 +++ b/vendor/github.com/asaskevich |
related to #69776 |
@ash2k the main concern would be "what was working before but will not work anymore", just identifying the possible scenarios, so we can figure out if we can tolerate it or need to document it or work around it. |
I think go-openapi added some changes that could be concerning for custom resource validation, but I'll double-check to be sure. Having said that, there are a few problems with the current vendored go-openapi that I know of. I'll be sending a fix upstream for those problems, and maybe we could update the vendored go-openapi then (or I'll do a bump again later if it takes time merging the upstream PR). |
Turns out #69627 updated
I'm not sure how to proceed with this PR 🤷♂️ If all tests pass then... how else can we ensure it is all good? |
6bb1822
to
60b371d
Compare
@ash2k great question :) How about enumerating what you saw? Thanks, |
Godeps/Godeps.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"ImportPath": "k8s.io/kubernetes", | |||
"GoVersion": "go1.10", | |||
"GoVersion": "go1.11", |
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.
I think it's ok to bump this version now, but maybe @ixdy can confirm.
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.
I think this field is automatically filled in by godep
based on what version of the runtime it's using. it's pretty meaningless.
@@ -1351,132 +1351,132 @@ | |||
}, | |||
{ | |||
"ImportPath": "github.com/docker/docker/api", | |||
"Comment": "docs-v1.12.0-rc4-2016-07-15-9510-ga9fbbdc8d", | |||
"Comment": "docs-v1.12.0-rc4-2016-07-15-9510-ga9fbbdc8dd", |
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.
Also @ixdy do you know why the extra d
is getting appended here? I have a vendor change that is doing the same thing and I don't know why.
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.
I have no idea how godep
decides how to truncate the git commit. it might be a git setting, it might be something else. godep
is terrible.
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.
I think it's just using git describe --tags
.
The manpage for git describe
notes
Note that the suffix you get if you type these commands today may be longer than what Linus saw above when he ran these commands, as your Git repository may have new commits whose object names begin with 975b that did not exist back then, and "-g975b" suffix alone may not be sufficient to disambiguate these commits.
so I guess this is WAI? 🤷♂️
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.
I remember discussing about this with @cblecker somewhere but can't find the issue now... 🤷♀️
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.
it has to do with the git version you're running locally. at a certain point, the default changed for git describe
see tools/godep#560 for more details. This PR to fix it was opened, but that happened after godep stopped active development.
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.
+1 to not include the changes that update the comment with just one more character or one less character for the SHA
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 could also just scrub all comments from the file. That's what we do with the staging-godeps script
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 probably should. This is getting stupid.
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.
+1 to scrub all comments from the file
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.
submit a PR to address this: #70687
To be clear, my concern is just that we don't unnecessarily expand the dependency list. I'm not super happy that we're importing mgo/bson after this change. I'm not sure there's anything to be done about it :( But every new dependency makes things a tiny bit worse for every consumer of kubernetes. Regressions should be caught by tests, I'm not worried about that. |
We use go-swagger to generate a client for one of our services. It generates code that uses diff --git a/vendor/github.com/go-openapi/runtime/client_operation.go b/vendor/github.com/go-openapi/runtime/client_operation.go
index fa21eacf3..421f726c5 100644
--- a/vendor/github.com/go-openapi/runtime/client_operation.go
+++ b/vendor/github.com/go-openapi/runtime/client_operation.go
@@ -14,11 +14,6 @@
package runtime
-import (
- "context"
- "net/http"
-)
-
// ClientOperation represents the context for a swagger operation to be submitted to the transport
type ClientOperation struct {
ID string
@@ -30,8 +25,6 @@ type ClientOperation struct {
AuthInfo ClientAuthInfoWriter
Params ClientRequestWriter
Reader ClientResponseReader
- Context context.Context
- Client *http.Client
}
// A ClientTransport implementor knows how to submit Request objects to some destination Regarding --- FAIL: TestHTTPSAddressValid (0.02s)
--- FAIL: TestHTTPSAddressValid/enusre_aws_sns_topic_endpoint_url_succeeds_on_'https://a.b-c.de' (0.00s)
<autogenerated>:1:
Error Trace: validation_test.go:172
Error: Received unexpected error:
Key: 'WubbaLubbaDubDub.Endpoint' Error:Field validation for 'Endpoint' failed on the 'aws_sns_topic_endpoint_url' tag
Test: TestHTTPSAddressValid/enusre_aws_sns_topic_endpoint_url_succeeds_on_'https://a.b-c.de'
--- FAIL: TestHTTPSAddressValid/enusre_aws_sns_topic_endpoint_url_succeeds_on_'https://a.b--c.de/' (0.00s)
<autogenerated>:1:
Error Trace: validation_test.go:172
Error: Received unexpected error:
Key: 'WubbaLubbaDubDub.Endpoint' Error:Field validation for 'Endpoint' failed on the 'aws_sns_topic_endpoint_url' tag
Test: TestHTTPSAddressValid/enusre_aws_sns_topic_endpoint_url_succeeds_on_'https://a.b--c.de/'
--- FAIL: TestHTTPSAddressValid/enusre_aws_sns_topic_endpoint_url_succeeds_on_'https://a.ba--c.de/' (0.00s)
<autogenerated>:1:
Error Trace: validation_test.go:172
Error: Received unexpected error:
Key: 'WubbaLubbaDubDub.Endpoint' Error:Field validation for 'Endpoint' failed on the 'aws_sns_topic_endpoint_url' tag
Test: TestHTTPSAddressValid/enusre_aws_sns_topic_endpoint_url_succeeds_on_'https://a.ba--c.de/'
--- FAIL: TestHTTPSAddressValid/enusre_aws_sns_topic_endpoint_url_succeeds_on_'https://a.xn--c.de/' (0.00s)
<autogenerated>:1:
Error Trace: validation_test.go:172
Error: Received unexpected error:
Key: 'WubbaLubbaDubDub.Endpoint' Error:Field validation for 'Endpoint' failed on the 'aws_sns_topic_endpoint_url' tag
Test: TestHTTPSAddressValid/enusre_aws_sns_topic_endpoint_url_succeeds_on_'https://a.xn--c.de/'
--- FAIL: TestHTTPSAddressValid/enusre_aws_sns_topic_endpoint_url_succeeds_on_'http://www.atlassian.com./' (0.00s)
<autogenerated>:1:
Error Trace: validation_test.go:172
Error: Received unexpected error:
Key: 'WubbaLubbaDubDub.Endpoint' Error:Field validation for 'Endpoint' failed on the 'aws_sns_topic_endpoint_url' tag
Test: TestHTTPSAddressValid/enusre_aws_sns_topic_endpoint_url_succeeds_on_'http://www.atlassian.com./'
FAIL Everything is fine if I use new version. There is a whole bunch of url validation issues that have been fixed (probably not an exhaustive list) that I found: asaskevich/govalidator#193 asaskevich/govalidator#150 asaskevich/govalidator#151 asaskevich/govalidator#194 asaskevich/govalidator#269 I've been thinking about this issue in general. Basically, here is a user story that kubernetes community probably needs to have an answer for: |
/lgtm |
/hold /assign @thockin |
(citing myself)
BTW, forgot to mention - I've built a tool to generate |
|
@thockin @justaugustus @nikhita @cblecker - fwiw, we do have 2 packages with the same license already. So at least it's not a new license.
|
Ok from the go-openapi side. 👍 |
I think I'm satisfied that this is useful. I'll approve if it is rebased. Sorry for taking a while. I think it's not adding new license types. |
github.com/go-openapi/* github.com/asaskevich/govalidator
60b371d
to
8763223
Compare
/lgtm |
/approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ash2k, lavalamp, thockin 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 |
@cblecker mind canceling the hold? |
/hold cancel |
What type of PR is this?
/kind cleanup
/sig api-machinery
What this PR does / why we need it:
I'm trying to use the exact same versions of libraries that Kubernetes uses and it turned out that a few of the dependencies are significantly older than the versions I use atm. I discovered that because a few of my tests started to fail because of "regression" - bugs were fixed in newer versions but not in the versions kubernetes uses. So I decided to update the kubernetes versions.
These are the dependencies with the issues that are resolved for me in newer versions:
github.com/go-openapi/* <- new fields in some structs were added which I need
github.com/asaskevich/govalidator <- supports stricter validation of URLs
github.com/ghodss/yaml <- bug with pointer struct unmarshallingDoes this PR introduce a user-facing change?: