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

[Blocked do not merge] ⚠️ (kustomize/v1,go/v3): upgrade kustomize 3.x to 4.x and add test to ensure backwards compatibility #2758

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Jun 19, 2022

Description

  • Update kustomize from 3x to 4x
  • Add script test to ensure that maintainers do not change the default scaffold of kustomize/v1 and go/v3 which are stable and we MUST ensure the backwards compatibility

IMPORTANT: The changes on the scaffolds to comply with the kustomize 4.x and remove what is deprecated must be done in the new kustomze/v2-alpha plugin (kubebuilder/pkg/plugins/common/kustomize/v2/) (examples as we did here)which will be used by a new golang plugin as go/v4-alpha. So that, we do not introduce breaking changes and do not affect those who scaffold the projects with Kubebuilder CLI 3.x and are trying to use its new versions. For further understanding of why it is required check the Plugin Versioning doc: https://book.kubebuilder.io/plugins/plugins-versioning.html

Motivation

Allow us to move forward and support Apple Silicon with go/v3 and kustomize/v1 without introducing breaking changes for those who scaffold the projects with old CLI versions.

BLOCKED BY

/hold

By testing the changes with its big consumer SDK we could find that the make bundle target on their project brokes:

```sh
$ make bundle
/Users/camilamacedo86/go/src/github.com/operator-framework/operator-sdk/testdata/go/v3/memcached-operator/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
operator-sdk generate kustomize manifests --interactive=false -q
cd config/manager && /Users/camilamacedo86/go/src/github.com/operator-framework/operator-sdk/testdata/go/v3/memcached-operator/bin/kustomize edit set image controller=controller:latest
/Users/camilamacedo86/go/src/github.com/operator-framework/operator-sdk/testdata/go/v3/memcached-operator/bin/kustomize build config/manifests | operator-sdk generate bundle -q --overwrite --version 0.0.1  
Error: remove operation does not apply: doc is missing path: "/spec/template/spec/containers/1/volumeMounts/0": missing value

So, we cannot move forward with this one for safety and for not to risk introducing breaking changes for the projects which consume Kubebuilder as lib. See that the kustomize documentation about what should no longer work when we move from v3 to v4 is not very clear.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 19, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2022
@camilamacedo86 camilamacedo86 mentioned this pull request Jun 19, 2022
4 tasks
@camilamacedo86 camilamacedo86 force-pushed the update-kustomize-version-stable branch from 6cfea18 to ddb867f Compare June 19, 2022 07:42
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 19, 2022
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Jun 19, 2022

About errors in the CI/prow:

Regards APIDiff / Verify API differences (pull_request)

That is OK. We have the check to not allow we break the API.
In this case the check fails because see that we bump the kustomze only
We can safely move forward here.

@camilamacedo86 camilamacedo86 force-pushed the update-kustomize-version-stable branch from ddb867f to acab8fa Compare June 19, 2022 07:53
@kubernetes-sigs kubernetes-sigs deleted a comment from k8s-ci-robot Jun 19, 2022
@kubernetes-sigs kubernetes-sigs deleted a comment from k8s-ci-robot Jun 19, 2022
@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-18-20

@camilamacedo86 camilamacedo86 added this to the 3.5.0 milestone Jun 19, 2022
@camilamacedo86 camilamacedo86 changed the title ⚠️ (kustomize/v1,go/v3): upgrade kustomize 3.x to 4x and add test to ensure backwords compability ⚠️ (kustomize/v1,go/v3): upgrade kustomize 3.x to 4.x and add test to ensure backwards compatibility Jun 19, 2022
@camilamacedo86 camilamacedo86 force-pushed the update-kustomize-version-stable branch 2 times, most recently from 92adf8c to 2716514 Compare June 19, 2022 08:45
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Looks good, just a minor spelling nit.

pkg/plugins/common/kustomize/v1/init.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 force-pushed the update-kustomize-version-stable branch from 2716514 to 199467e Compare June 19, 2022 12:14
@FillZpp
Copy link

FillZpp commented Jun 20, 2022

The macos-latest unit-test failed because of Github rate-limiter failed the request. Either authenticate or wait a couple of minutes., should we re-run it or just ignore?

@camilamacedo86
Copy link
Member Author

Hi @FillZpp,

The macos-latest unit-test failed because of Github rate-limiter failed the request. Either authenticate or wait a couple of minutes., should we re-run it or just ignore?

We should not ignore this test.
It was a flake and I re-run the test which is passing now.
Thank you

@FillZpp
Copy link

FillZpp commented Jun 20, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2022
@camilamacedo86 camilamacedo86 force-pushed the update-kustomize-version-stable branch from 199467e to 626691a Compare June 20, 2022 13:04
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2022
@camilamacedo86 camilamacedo86 force-pushed the update-kustomize-version-stable branch 2 times, most recently from 2d883f0 to cb42506 Compare June 20, 2022 13:25
@camilamacedo86 camilamacedo86 force-pushed the update-kustomize-version-stable branch from cb42506 to 45bae2c Compare June 20, 2022 13:27
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Looks good!

@rashmigottipati
Copy link
Contributor

/lgtm
/hold to allow addressing of nits from reviews

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2022
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 21, 2022
Copy link
Contributor

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

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

Also, it looks like the Kustomize version in scaffolds is not updated to v4.5.5 from v3.5.4 which is causing the APIDiff test to fail.

@camilamacedo86
Copy link
Member Author

Hi @rashmigottipati,

Also, it looks like the Kustomize version in scaffolds is not updated to v4.5.5 from v3.5.4 which is causing the APIDiff test to fail.

The APIDiff comply with is expected in this case. In this scenarios, the bot does not merge alone we need to force

Copy link
Contributor

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, everettraven, rashmigottipati

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rashmigottipati
Copy link
Contributor

/test pull-kubebuilder-e2e-k8s-1-18-20

1 similar comment
@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-18-20

@camilamacedo86 camilamacedo86 force-pushed the update-kustomize-version-stable branch from 76884fd to 4015d37 Compare June 22, 2022 06:09
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2022
@camilamacedo86
Copy link
Member Author

/hold

@camilamacedo86 camilamacedo86 changed the title ⚠️ (kustomize/v1,go/v3): upgrade kustomize 3.x to 4.x and add test to ensure backwards compatibility [Blocked do not merge] ⚠️ (kustomize/v1,go/v3): upgrade kustomize 3.x to 4.x and add test to ensure backwards compatibility Jun 23, 2022
@camilamacedo86 camilamacedo86 removed this from the 3.5.0 milestone Jun 23, 2022
@camilamacedo86
Copy link
Member Author

I am closing this one and we can reopen if we check that we can move forward here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants