-
Notifications
You must be signed in to change notification settings - Fork 531
chore: upgrade kubernetes dependencies #1301
chore: upgrade kubernetes dependencies #1301
Conversation
Welcome @sagikazarmark! |
bff1574
to
fc6d2b5
Compare
Hm, can't see any obvious reason why the tests are breaking... |
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.
Thank you for this. Please fix the kind image and, if you don't mind, update to v0.19.3 dependencies, as well as running go mod tidy
please to try to keep go.sum
in a reasonable state.
0bc9aca
to
6a7421b
Compare
@jimmidyson thanks for the review. I addressed/replied to your comments. I actually ran |
@sagikazarmark I was surprised there weren't a lot more removals from |
/approve |
I'll take a look at it. My guess is there is a transitive dependency on an earlier Kubernetes version. |
Controller runtime for instance still relies on 1.18.6 |
@sagikazarmark Ah ok that makes sense |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jimmidyson, makkes, sagikazarmark 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 |
@sagikazarmark Sorry for the bother but would you mind rebasing this against master and dropping the last two commits in this branch (only need the updated kubernetes dependencies)? Hopefully that should sort out the CI issues. |
a205468
to
fc6d2b5
Compare
fc6d2b5
to
e3eecef
Compare
@jimmidyson I've just rebased it, looks like at the same time you did. 😄 |
Oh I'm sorry @sagikazarmark, I was just trying to help out to get it in, hope that's ok. I'm going to push another commit just to bump the dependencies to latest now they're published, then we can get this merged. Thank you for kicking this upgrade off. |
Sure, no worries. 😄 Let me know if I can help. Thanks! |
e3eecef
to
8430e73
Compare
Looks like it fails with the same error 😕 |
Signed-off-by: Mark Sagi-Kazar <[email protected]>
8430e73
to
223b45a
Compare
@sagikazarmark My dodgy rebase - I knew I should have waited for you 😉 |
Hm, I'm not sure, the rebase looks fine to me. Only |
Yeah it seems to be a consistent test failure. |
To test the one failing test you can run (after spinning up some clusters and deploying kubefed - $ make bin/e2e
$ bin/e2e -kubeconfig=${HOME}/.kube/config -ginkgo.v -single-call-timeout=1m -ginkgo.trace -ginkgo.randomizeAllSpecs -ginkgo.focus="deployments.apps.* should be created, read, updated and deleted successfully" I'm also looking at this btw so hope we can get this fixed quickly and merged. |
I can see a bunch of |
Yeah it looks like a bug in the jsonpatch upgrade (test below). package a
import (
"encoding/json"
"fmt"
"testing"
jsonpatch "github.com/evanphx/json-patch"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"sigs.k8s.io/kubefed/pkg/controller/util"
)
func TestPatch(t *testing.T) {
orig := &unstructured.Unstructured{
Object: map[string]interface{}{
"metadata": map[string]interface{}{
"annotations": map[string]interface{}{
"deployment.kubernetes.io/revision": "1",
},
},
},
}
origJSON, _ := orig.MarshalJSON()
overrides := util.ClusterOverrides{{
Op: "add",
Path: "/metadata/annotations",
Value: map[string]string{
"foo": "bar",
},
}, {
Path: "/metadata/annotations/foo",
Op: "remove",
}}
jsonPatchBytes, _ := json.Marshal(overrides)
patch, _ := jsonpatch.DecodePatch(jsonPatchBytes)
patchedObjectJSONBytes, _ := patch.Apply(origJSON)
fmt.Println(string(patchedObjectJSONBytes))
} |
Or potentially a fix in the JSONPatch library... I think this is it tbh. |
Ah yes this is because the override is wrong... fixing now. |
OK fixed. The override was actually invalid... |
/lgtm |
Thanks @sagikazarmark! |
Thanks for the help @jimmidyson |
Signed-off-by: Mark Sagi-Kazar [email protected]
What this PR does / why we need it:
Upgrades the Kubernetes libraries to 1.19.2
Keeping libraries up to date is generally a good idea. I'm not aware of any major changes that might break backwards compatibility.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
The logr package needed a manual update because of container-runtime. Once 0.7.0 is out, we don't need to manually pin the logr library version.
Some other libraries have also been updated because they are also transitive dependencies.