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.

Sign up for GitHub

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

Jump to bottom

Update controller tools v0.6.2 #17596

Merged
merged 2 commits into from Oct 18, 2021
Merged

Update controller tools v0.6.2 #17596

merged 2 commits into from Oct 18, 2021

Conversation

jrajahalme
Copy link
Member

Update controller-tools to v0.6.2 to gain support for opaque json in k8s custom resources. This is needed for including Envoy resources in the upcoming Cilium Envoy Config CRD.

This update pulled in a Ginkgo update which required some CI updates. These are separated out into a commit of their own to make reviewing easier.

@jrajahalme jrajahalme added area/CI Continuous Integration testing issue or flake sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/misc This PR makes changes that have no direct user impact. labels Oct 13, 2021
@jrajahalme jrajahalme requested review from a team as code owners October 13, 2021 13:47
@jrajahalme jrajahalme requested a review from a team October 13, 2021 13:47
@jrajahalme jrajahalme requested a review from a team as a code owner October 13, 2021 13:47
@jrajahalme jrajahalme mentioned this pull request Oct 13, 2021
Closed
9 tasks
christarazi
christarazi approved these changes Oct 13, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Thanks for separating this out and taking care of the controller-tools update. LGTM.

@aanm aanm added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 13, 2021
@aanm
Copy link
Member

aanm commented Oct 13, 2021

@jrajahalme needs rebase

joestringer
joestringer approved these changes Oct 13, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

The new cmdrefs for completion seem a bit unnecessary but I'm assuming that it's all just part of the tool-based autogeneration so sure 👍 LGTM from docs-structure perspective.

The ginkgo changes looked pretty confusing at first glance so I'm assuming the other reviewers took a closer look.

@jrajahalme jrajahalme force-pushed the update-controller-tools-v0.6.2 branch from 9c68daf to f6b6573 Compare October 14, 2021 11:23
@jrajahalme
Copy link
Member Author

The ginkgo changes looked pretty confusing at first glance so I'm assuming the other reviewers took a closer look.

We'll have to see the CI. runs to know if the Ginkgo changes are complete and if our test suite is compatible with this Ginkgo version... 🤞

@jrajahalme
Copy link
Member Author

jrajahalme commented Oct 14, 2021 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: kube-dns was not able to get into ready state

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.21-kernel-4.9' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sConformance Portmap Chaining Check connectivity-check compliance with portmap chaining

Failure Output

FAIL: connectivity-check pods are not ready after timeout

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.9 so I can create a new GitHub issue to track it.

@jrajahalme
Copy link
Member Author

All the test fails seem wholly unrelated to these changes, which are:

  1. Fix Ginkgo scope selection for multiple focus args by determining the scope from the first focus arg and by deferring scope processing to the returned function. Successful Goinkgo runs show that this works as expected.
  2. New version of k8s code generation. The code compiles and code generation checks do not flag any differences in the generated code as freshly generated and included in this PR. So this piece is working as well.

Based on the above I'm labeling this PR as ready-to-merge.

@jrajahalme jrajahalme added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 15, 2021
@jrajahalme
Copy link
Member Author

jrajahalme commented Oct 15, 2021 •
edited by maintainer-s-little-helper bot

/mlh new-flake Cilium-PR-K8s-GKE

👍 created #17617

@jrajahalme
Copy link
Member Author

jrajahalme commented Oct 15, 2021 •
edited by maintainer-s-little-helper bot

/mlh new-flake Cilium-PR-K8s-1.21-kernel-4.9

👍 created #17618

nbusseneau
nbusseneau approved these changes Oct 15, 2021
View reviewed changes
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Code changes LGTM.

@nbusseneau
Copy link
Member

nbusseneau commented Oct 15, 2021 •
edited

All the test fails seem wholly unrelated to these changes, which are:

1. Fix Ginkgo scope selection for multiple focus args by determining the scope from the first focus arg and by deferring scope processing to the returned function. Successful Goinkgo runs show that this works as expected.

2. New version of k8s code generation. The code compiles and code generation checks do not flag any differences in the generated code as freshly generated and included in this PR. So this piece is working as well.

Based on the above I'm labeling this PR as ready-to-merge.

Shouldn't we wait for other reviewers to approve? 🤔

Also re: flakes => I think we should really try to either block or disable when that happens /cc @twpayne

@pchaigno pchaigno removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Oct 15, 2021
@pchaigno
Copy link
Member

Shouldn't we wait for other reviewers to approve? thinking

@nbusseneau All requested team reviews are already covered. Nathan and Robin's reviews were required for cilium/kubernetes and cilium/vendor respectively. Those two teams are already covered by Chris and André.

@jrajahalme
ginkgo: Update to 1.16.4
b4ff4e8 
The following controller-tools update pulls in a new ginkgo version
with a deferred Describe() change that is incompatible with the Cilium
Ginkgo suite handling of scopes. Separate this change and fix to this
commit to make it clear what is going on.

To properly deal with deferred Describe() handling of Ginkgo, we also
need to defer the scope handling by moving it to the function body
called by Ginkgo.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
k8s: Update controller-tools to v0.6.2
5587b15 
Update controller tools to get object-level support for unknown
fields. This is needed for Envoy CRDs. Still using the fork at
https://github.com/christarazi/controller-tools/tree/v0.6.2

test: Fix ginkgo breakage due to new version

Documentation: Add completion files due to spf13/cobra update

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme force-pushed the update-controller-tools-v0.6.2 branch from f6b6573 to 5587b15 Compare October 18, 2021 11:02
@jrajahalme
Copy link
Member Author

@joamaki asked for a rebase & retest, so here we go :-)

@joamaki
Copy link
Contributor

joamaki commented Oct 18, 2021 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.19-kernel-5.4' hit: #17353 (90.54% similarity)

@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Oct 18, 2021
Closed
@joamaki joamaki merged commit 36126dc into cilium:master Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@christarazi christarazi christarazi approved these changes

@nbusseneau nbusseneau nbusseneau approved these changes

@aanm aanm aanm approved these changes

@nathanjsweet nathanjsweet Awaiting requested review from nathanjsweet

@rolinh rolinh Awaiting requested review from rolinh rolinh was automatically assigned from cilium/vendor

Assignees

@nathanjsweet nathanjsweet

@rolinh rolinh

Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@jrajahalme @aanm @nbusseneau @pchaigno @joamaki @joestringer @christarazi @nathanjsweet @rolinh