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

🐛 (kustomize/v2, go/v4): fix issue to don't set kustomizeconfig.yaml when Webhooks are not enable into the project #3585

Closed
wants to merge 2 commits into from

Conversation

lowang-bh
Copy link
Member

@lowang-bh lowang-bh commented Sep 4, 2023

Description

Fix error in the layout when webhooks are not enable in the project.

Motivation

Fixes #3367

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 4, 2023
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 4, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @lowang-bh. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

camilamacedo86

This comment was marked as outdated.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 6, 2023
@lowang-bh

This comment was marked as resolved.

camilamacedo86

This comment was marked as resolved.

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 7, 2023
@lowang-bh

This comment was marked as resolved.

@Sajiyah-Salat

This comment was marked as off-topic.

@camilamacedo86 camilamacedo86 changed the title ⚠ fix issue #3367: don't gen Webhooks when project has no webhook enable 🐛 fix issue #3367: don't gen Webhooks when project has no webhook enable Sep 12, 2023
camilamacedo86

This comment was marked as resolved.

@lowang-bh

This comment was marked as resolved.

@lowang-bh
Copy link
Member Author

Can I ask on which part you are currently working. Can I help you in this?

Still need a new test to ensure that the project works fine when no webhooks are enable

@Sajiyah-Salat

This comment was marked as off-topic.

@camilamacedo86 camilamacedo86 changed the title 🐛 fix issue #3367: don't gen Webhooks when project has no webhook enable 🐛 (issue #3367) don't set kustomizeconfig.yaml when Webhooks are not enable into the project Sep 13, 2023
camilamacedo86

This comment was marked as resolved.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 13, 2023
@camilamacedo86

This comment was marked as resolved.

@camilamacedo86

This comment was marked as resolved.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2023
@camilamacedo86
Copy link
Member

/pull-kubebuilder-e2e-k8s-1-26-6

@camilamacedo86

This comment was marked as resolved.

@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 13, 2023
@k8s-ci-robot

This comment was marked as resolved.

@lowang-bh
Copy link
Member Author

/retest

@camilamacedo86

This comment was marked as resolved.

@lowang-bh
Copy link
Member Author

As a follow up we will also need to do like here: #3629

Could you please check this PR and either give your thoughts.

What shall I do now?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2023
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 30, 2023
@lowang-bh

This comment was marked as outdated.

camilamacedo86 pushed a commit to camilamacedo86/kubebuilder that referenced this pull request Oct 1, 2023
… and projects without webhooks

At present, we scaffold config/crd/patches, kustomizations, and CA injections for every CRD, irrespective of whether webhooks are enabled in the project or not. However, these configurations are only relevant and valid if the project has webhooks.

Consequently, for projects without webhooks, this leads to failures as documented in kubebuilder pull request kubernetes-sigs#3585.

To address this, we are now introducing a test to ensure that projects without enabled webhooks function correctly and as anticipated.

Signed-off-by: Camila Macedo <[email protected]>
Co-authored-by: lowang-bh <[email protected]>

fix: just generate crd webhooks patches and ca injestions when webhooks are created for those
camilamacedo86 pushed a commit to camilamacedo86/kubebuilder that referenced this pull request Oct 1, 2023
… and projects without webhooks

At present, we scaffold config/crd/patches, kustomizations, and CA injections for every CRD, irrespective of whether webhooks are enabled in the project or not. However, these configurations are only relevant and valid if the project has webhooks.

Consequently, for projects without webhooks, this leads to failures as documented in kubebuilder pull request kubernetes-sigs#3585.

To address this, we are now introducing a test to ensure that projects without enabled webhooks function correctly and as anticipated.

Signed-off-by: Camila Macedo <[email protected]>
Co-authored-by: lowang-bh <[email protected]>

fix: just generate crd webhooks patches and ca injestions when webhooks are created for those
camilamacedo86 pushed a commit to camilamacedo86/kubebuilder that referenced this pull request Oct 1, 2023
… and projects without webhooks

At present, we scaffold config/crd/patches, kustomizations, and CA injections for every CRD, irrespective of whether webhooks are enabled in the project or not. However, these configurations are only relevant and valid if the project has webhooks.

Consequently, for projects without webhooks, this leads to failures as documented in kubebuilder pull request kubernetes-sigs#3585.

To address this, we are now introducing a test to ensure that projects without enabled webhooks function correctly and as anticipated.

Signed-off-by: Camila Macedo <[email protected]>
Co-authored-by: lowang-bh <[email protected]>
Comment on lines +22 to +23
# configurations:
# - kustomizeconfig.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# configurations:
# - kustomizeconfig.yaml
#configurations:
#- kustomizeconfig.yaml

We cannot have the space
Also, we should automatically uncomment this one when a webhook is scaffold.

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Hi @lowang-bh,

By doing the e2e tests I could check that it would still missing few things
So, I used your PR to give your the credit as well and keep you as co-author
See; #3647

Can we close this one in favor of #3647
Could you please help in the review of #3647 for we move forward and get that one merged.

/approved cancel
/lgtm cancel

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lowang-bh
Once this PR has been reviewed and has the lgtm label, please ask for approval from camilamacedo86. For more information see the Kubernetes Code Review Process.

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

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2023
@lowang-bh lowang-bh closed this Oct 4, 2023
camilamacedo86 pushed a commit to camilamacedo86/kubebuilder that referenced this pull request Oct 10, 2023
… and projects without webhooks

At present, we scaffold config/crd/patches, kustomizations, and CA injections for every CRD, irrespective of whether webhooks are enabled in the project or not. However, these configurations are only relevant and valid if the project has webhooks.

Consequently, for projects without webhooks, this leads to failures as documented in kubebuilder pull request kubernetes-sigs#3585.

To address this, we are now introducing a test to ensure that projects without enabled webhooks function correctly and as anticipated.

Signed-off-by: Camila Macedo <[email protected]>
Co-authored-by: lowang-bh <[email protected]>
Lauquik pushed a commit to Lauquik/kubebuilder that referenced this pull request Oct 13, 2023
… and projects without webhooks

At present, we scaffold config/crd/patches, kustomizations, and CA injections for every CRD, irrespective of whether webhooks are enabled in the project or not. However, these configurations are only relevant and valid if the project has webhooks.

Consequently, for projects without webhooks, this leads to failures as documented in kubebuilder pull request kubernetes-sigs#3585.

To address this, we are now introducing a test to ensure that projects without enabled webhooks function correctly and as anticipated.

Signed-off-by: Camila Macedo <[email protected]>
Co-authored-by: lowang-bh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webhooks manifests has been scaffolded when a project has not webhooks created
4 participants