-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
⚠️ : (kustomize/v2, go/v4): Stop to scaffold CA patch injection since it is unnecessary #3555
⚠️ : (kustomize/v2, go/v4): Stop to scaffold CA patch injection since it is unnecessary #3555
Conversation
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 a lot for your contribution 🥇
Following some notes and recommendations.
Please, note that when you do a change you must run make generate
to generate the samples.
Also, please be aware that after your change it must pass in the e2e tests so that we can ensure that all still working fine as expected.
By last, the emoji is wrong because it is a change that affects the users. In this case it seems that the best one would to be :bug:
or :warning:
.
Also, can you update the title for a clear explanation about the change since it is used to generate the release notes? Example: :bug: (kustomize/v2, go/v4): Stop to scaffold CA patch injection since it is unnecessary
Hmm I'm having some issue running What I'm seeing is this: ❯ git status
On branch master
Your branch is up to date with 'origin/master'.
nothing to commit, working tree clean
❯ make generate
rm -rf testdata/
./test/testdata/generate.sh
Building kubebuilder
~/workspace/kubebuilder/testdata/project-v2 ~/workspace/kubebuilder
rm: cannot remove '/usr/local/bin/kustomize': Permission denied
make: *** [Makefile:72: generate-testdata] Error 1 |
Hi @lentzi90, It seems that the script is unable to cleanup/remove all from the dir testdata before moveforward. Run Also, please ensure that you have your branch rebase with master changes as well. |
fdd6fc3
to
c0f04db
Compare
Interesting... I had to do sudo rm -rf testdata
sudo rm -rf docs/book/src/component-config-tutorial/testdata/project/bin
sudo rm -rf docs/book/src/multiversion-tutorial/testdata/project/bin to get a successful |
c0f04db
to
79165fb
Compare
Ok so it is complaining that the API changed. I can understand that, but I'm not sure how to solve it. Should we keep the API but make it a no-op? |
244c496
to
6d06a85
Compare
Hmm I got a clean e2e test locally before pushing this 🤔 |
Alright so it somehow was a flake. Weird. Then the only remaining question from my side is this:
|
Kubebuilder is a LIB as well. So, it is saying that the change broke the kubebuilder API because the option no longer exist.
That is fine we can move forward in this case because it does not seems break endusers. I am updating the title accordingly |
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 seems make sense for me and the changes shows fine.
It does not seems a breaking change for those who already scaffold a project with the old versions.
So I am /lgtm
But I also would like to get feedback from others from the community
Let's see if others can help either.
Thx
The CA injection annotation is now unnecessary since switching from vars to replacements in the kustomization. The replacements have "create: true" which means that they will add the annotation if it is not already there, hence the patch is not needed.
6d06a85
to
9309399
Compare
I also met this error. I try to add write permission to this dir and then works. You can have a try. |
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.
/approved cancel
We will need to check it further
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lentzi90 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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 raising it.
Upon reviewing your PR (#3555), it seems that the root of the issue might lie in the replacements. We must consider genuine scenarios where users might want to decide which resource should have the cert-manager injected.
Rather than removing the injections altogether, it might be more appropriate to address the replacements. Our goal should be to ensure that the certmanager is only injected into the uncommented files.
Would you like to help us to sorting it out by changing the replacement to ensure that we will ONLY inject the cert-manager in the CRDs and/or webhooks which are uncommented?
Please, see the comment for further information: #3538 (comment)
Thanks for the explanation! I commented on the issue also. I will give it some thought, although I don't have an immediate solution to this. If I come up with something I will try to implement it and push here |
@lentzi90: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
This patch is no longer needed since it is injected by the replacements anyway.
Fixes #3538