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

🐛 (go/v4,kustomize/v2): Fix problems by simplify scaffold and removing webhookcainjection_patch. Clarifying replacements for cert-manager #4123

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

🐛 (go/v4,kustomize/v2): Fix problems by simplify scaffold and removing webhookcainjection_patch. Clarifying replacements for cert-manager #4123

wants to merge 1 commit into from

Conversation

camilamacedo86
Copy link
Member

  • Removed config/default/webhookcainjection_patch.yaml to streamline the scaffold.
  • Clarified replacements blocks in kustomization.yaml for easier understanding. Each block is now labeled with instructions for uncommenting based on specific webhook scenarios (ValidatingWebhook, DefaultingWebhook, ConvertingWebhook).

Closes: #3538
Closes: #4119

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 31, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title ✨ Simplify scaffold by removing webhookcainjection_patch and clarifying replacements ✨ (go/v4,kustomize/v2): Simplify scaffold by removing webhookcainjection_patch and clarifying replacements Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title ✨ (go/v4,kustomize/v2): Simplify scaffold by removing webhookcainjection_patch and clarifying replacements ✨ (go/v4,kustomize/v2): Simplify scaffold by removing webhookcainjection_patch and clarifying replacements for cert-manager Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title ✨ (go/v4,kustomize/v2): Simplify scaffold by removing webhookcainjection_patch and clarifying replacements for cert-manager 🐛 : (go/v4,kustomize/v2): fix issues by simplify scaffold by removing webhookcainjection_patch and clarifying replacements for cert-manager Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 : (go/v4,kustomize/v2): fix issues by simplify scaffold by removing webhookcainjection_patch and clarifying replacements for cert-manager 🐛 : (go/v4,kustomize/v2): fix issues by simplify scaffold. Removes webhookcainjection_patch and clarifying replacements for cert-manager Aug 31, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 : (go/v4,kustomize/v2): fix issues by simplify scaffold. Removes webhookcainjection_patch and clarifying replacements for cert-manager 🐛 : (go/v4,kustomize/v2): Fix problems by simplify scaffold and removing webhookcainjection_patch. Clarifying replacements for cert-manager Aug 31, 2024
@camilamacedo86 camilamacedo86 force-pushed the simplify-replaces branch 3 times, most recently from 8d61421 to 146865f Compare August 31, 2024 21:41
@camilamacedo86 camilamacedo86 changed the title 🐛 : (go/v4,kustomize/v2): Fix problems by simplify scaffold and removing webhookcainjection_patch. Clarifying replacements for cert-manager 🐛 (go/v4,kustomize/v2): Fix problems by simplify scaffold and removing webhookcainjection_patch. Clarifying replacements for cert-manager Aug 31, 2024
@camilamacedo86 camilamacedo86 mentioned this pull request Sep 1, 2024
Closed
varshaprasad96
varshaprasad96 reviewed Sep 3, 2024
View reviewed changes
testdata/project-v4-multigroup-with-deploy-image/config/default/kustomization.yaml Outdated
# - select:
# kind: ValidatingWebhookConfiguration
# fieldPaths:
# - .metadata.annotations.[cert-manager.io/inject-ca-from]
Copy link
Member

Choose a reason for hiding this comment

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

@camilamacedo86 one question in this - replacements are executed sequentially. If we enable all defaulting, mutating and conversion - wouldn't the annotation be overwritten given the field path is the same?

Copy link
Member Author

@camilamacedo86 camilamacedo86 Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In our e2e tests, we have enabled all of them. You can see the changes here: Link to GitHub PR (we uncommented all tests, and they work as expected).

Additionally, the paths are different for each configuration. One will replace the ValidatingWebhookConfiguration, another will replace the MutatingWebhookConfiguration, and so on.

Moreover, in the e2e tests for the deploy image sample, we are specifically checking the scenario where only validating webhooks are scaffolded. You can review that here: Link to GitHub Workflow.

However, If you identify any scenarios where this could potentially cause issues, please let us know. We can then add e2e tests to ensure everything is functioning correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

/hold

We are working to improve the coverage /tests so that we can ensure this one better

Copy link
Member Author

Choose a reason for hiding this comment

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

/hold cancel

Now, see that we are testing with e2e all samples under testdata and the tutorial ones
Also, for go/v4 we added a test to validate webhooks when they are applied in a ns that is not the same where the operator and its server is running

So, it is very well covered too.

@camilamacedo86 camilamacedo86 force-pushed the simplify-replaces branch 2 times, most recently from a7347f4 to f4df3c6 Compare September 3, 2024 07:15
@camilamacedo86 camilamacedo86 mentioned this pull request Sep 9, 2024
Closed
@camilamacedo86 camilamacedo86 force-pushed the simplify-replaces branch 2 times, most recently from 0ed05fd to bd3991f Compare September 10, 2024 18:57
@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 Sep 10, 2024
@camilamacedo86 camilamacedo86 force-pushed the simplify-replaces branch 2 times, most recently from 495d670 to d0fd88b Compare September 12, 2024 14:04
@camilamacedo86 camilamacedo86 mentioned this pull request Sep 13, 2024
Merged
@camilamacedo86 camilamacedo86 force-pushed the simplify-replaces branch 3 times, most recently from b252b3b to 2ae2852 Compare September 14, 2024 16:41
@camilamacedo86 camilamacedo86 mentioned this pull request Sep 14, 2024
Merged
@camilamacedo86
fix issues by simplifying the scaffold. Removes webhookcainjection_pa…
ccce3ae 
…tch and clarify replacements.

- Removed config/default/webhookcainjection_patch.yaml to streamline the scaffold.
- Clarified replacements blocks in kustomization.yaml for easier understanding. Each block is now labeled with instructions for uncommenting based on specific webhook scenarios (ValidatingWebhook, DefaultingWebhook, ConvertingWebhook).
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 15, 2024
@camilamacedo86
Copy link
Member Author

c/c @lentzi90

@camilamacedo86
Copy link
Member Author

Hi @erikgb

You might be a good person to help us with the review of this one.
WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kavinjsir Kavinjsir Awaiting requested review from Kavinjsir

@rashmigottipati rashmigottipati Awaiting requested review from rashmigottipati

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
3 participants
@camilamacedo86 @k8s-ci-robot @varshaprasad96