🐛 (kustomize/v1 and kustomize/v2-alpha) : ComponentConfig scaffolds should not be done by default #2826

laxmikantbpandhare · 2022-07-13T23:13:26Z

A description of the change

This PR will verify whether the --component-config flag is passed or not. If not then it will not scaffold out the controller_manager_config.yaml file.

The motivation for the change

it fixes #2782

Tests done to ensure the changes:

Init command ( without the --component-config flag)

From this PR:

laxmikantbhaskarpandhare@lpandhar-mac test % kubebuilder init --domain my.domain --repo my.domain/guestbook

Writing kustomize manifests for you to edit...
false
Writing scaffold for you to edit...
Get controller runtime:
$ go get sigs.k8s.io/controller-runtime@v0.12.2
Update dependencies:
$ go mod tidy
Next: define a resource with:
$ kubebuilder create api
laxmikantbhaskarpandhare@lpandhar-mac test % tree
.
├── Dockerfile
├── Makefile
├── PROJECT
├── README.md
├── config
│   ├── default
│   │   ├── kustomization.yaml
│   │   ├── manager_auth_proxy_patch.yaml
│   │   └── manager_config_patch.yaml
│   ├── manager
│   │   ├── kustomization.yaml
│   │   └── manager.yaml
│   ├── prometheus
│   │   ├── kustomization.yaml
│   │   └── monitor.yaml
│   └── rbac
│       ├── auth_proxy_client_clusterrole.yaml
│       ├── auth_proxy_role.yaml
│       ├── auth_proxy_role_binding.yaml
│       ├── auth_proxy_service.yaml
│       ├── kustomization.yaml
│       ├── leader_election_role.yaml
│       ├── leader_election_role_binding.yaml
│       ├── role_binding.yaml
│       └── service_account.yaml
├── go.mod
├── go.sum
├── hack
│   └── boilerplate.go.txt
└── main.go

6 directories, 24 files

Init command ( with the changes and passed the --component-config flag)

laxmikantbhaskarpandhare@lpandhar-mac testData % kubebuilder init --domain my.domain --repo my.domain/guestbook --component-config

Writing kustomize manifests for you to edit...
true
Writing scaffold for you to edit...
Get controller runtime:
$ go get sigs.k8s.io/controller-runtime@v0.12.2
Update dependencies:
$ go mod tidy
Next: define a resource with:
$ kubebuilder create api
laxmikantbhaskarpandhare@lpandhar-mac testData % tree
.
├── Dockerfile
├── Makefile
├── PROJECT
├── README.md
├── config
│   ├── default
│   │   ├── kustomization.yaml
│   │   ├── manager_auth_proxy_patch.yaml
│   │   └── manager_config_patch.yaml
│   ├── manager
│   │   ├── controller_manager_config.yaml
│   │   ├── kustomization.yaml
│   │   └── manager.yaml
│   ├── prometheus
│   │   ├── kustomization.yaml
│   │   └── monitor.yaml
│   └── rbac
│       ├── auth_proxy_client_clusterrole.yaml
│       ├── auth_proxy_role.yaml
│       ├── auth_proxy_role_binding.yaml
│       ├── auth_proxy_service.yaml
│       ├── kustomization.yaml
│       ├── leader_election_role.yaml
│       ├── leader_election_role_binding.yaml
│       ├── role_binding.yaml
│       └── service_account.yaml
├── go.mod
├── go.sum
├── hack
│   └── boilerplate.go.txt
└── main.go

6 directories, 25 files

This will verify that it is working, the controller_manager_config.yaml file got scaffolded only when a flag(--component-config) is passed.

k8s-ci-robot · 2022-07-13T23:13:34Z

Hi @laxmikantbpandhare. 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.

laxmikantbpandhare · 2022-07-13T23:15:12Z

@camilamacedo86 - I verified the field --component-config and then scaffolded accordingly. I have a quick question, I did these changes in v1. Is there an expectation to do it in v2-alpha as well?

camilamacedo86 · 2022-07-14T19:04:24Z

Hi @laxmikantbpandhare,

@camilamacedo86 - I verified the field --component-config and then scaffolded accordingly. I have a quick question, I did these changes in v1. Is there an expectation to do it in v2-alpha as well?

Terrific ! yes, we need to also fix the v2-alpha kustomize plugin.

camilamacedo86 · 2022-07-14T19:04:42Z

/ok-to-test

laxmikantbpandhare · 2022-07-14T19:06:34Z

Hi @laxmikantbpandhare,

@camilamacedo86 - I verified the field --component-config and then scaffolded accordingly. I have a quick question, I did these changes in v1. Is there an expectation to do it in v2-alpha as well?

Terrific ! yes, we need to also fix the v2-alpha kustomize plugin.

OK, I will update that too and then will do the squash of the commits.

laxmikantbpandhare · 2022-07-14T23:06:28Z

@camilamacedo86 - I went through the tests that are failing and found that they are failing in kustomize build

It is because kustomization.yaml file as shown below, it required controller_manager_config.yaml file to be present. But as we did the change, that only allow that file to be generated when component-config got passed.

resources:
- manager.yaml

generatorOptions:
  disableNameSuffixHash: true

configMapGenerator:
- name: manager-config
  files:
  - controller_manager_config.yaml

The solution for this is, that I need to modify the test cases and enable the component-config field so that the controller_manager_config.yaml file will be generated WDYT?

pkg/plugins/common/kustomize/v1/scaffolds/init.go

test/e2e/deployimage/plugin_cluster_test.go

test/e2e/v4/generate_test.go

laxmikantbpandhare · 2022-07-19T15:59:11Z

/label tide/merge-method-squash

camilamacedo86 · 2022-07-20T07:45:46Z

Hi @laxmikantbpandhare,

Regards the comment : #2826 (comment)

That means:

We should ONLY add to the default kustomization the content which is relevant to the component config when the flag is used (So we also need to address this one)
We also need to add this step to the documents for those that want to use the component-config on projects that were not init with the flag
We need to have a new test on the e2e tests for v3 and v4 that verifies a project scaffold with component-config

Could you look to address these needs? WDYT?

laxmikantbpandhare · 2022-07-20T17:07:18Z

Hi @laxmikantbpandhare,

Regards the comment : #2826 (comment)

That means:

We should ONLY add to the default kustomization the content which is relevant to the component config when the flag is used (So we also need to address this one)

We also need to add this step to the documents for those that want to use the component-config on projects that were not init with the flag

We need to have a new test on the e2e tests for v3 and v4 that verifies a project scaffold with component-config

Could you look to address these needs? WDYT?

@camilamacedo86 - Got your point and agree with the suggestions. I will update the PR soon.

SayakMukhopadhyay · 2022-07-21T11:21:07Z

Hi @camilamacedo86 I am trying to align my own project to incorporate these changes manually (since its already scaffolded). A clarification regarding the last comment which might be important for this PR too

We should ONLY add to the default kustomization the content which is relevant to the component config when the flag is used

Is the generatorOptions required when creating the project without the config flag. In other words, without the flag should the kustomization.yaml be

resources:
- manager.yaml

or

resources:
- manager.yaml

generatorOptions:
  disableNameSuffixHash: true

I am pretty sure that all of

configMapGenerator:
- name: manager-config
  files:
  - controller_manager_config.yaml

needs to be removed though.

SayakMukhopadhyay

I dont know if this is something that you intend to fix but just thought to mention that the ManagerConfigPatch is also something that should depend on whether the config flag is pased or not. The generated patch is referencing the controller_manager_config.yaml which won't exist and is sure to confuse users.
https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/common/kustomize/v1/scaffolds/internal/templates/config/kdefault/manager_config_patch.go

Also notice how the kustomize.yaml referencing the above patch is commenting out the line to include the patch

kubebuilder/pkg/plugins/common/kustomize/v1/scaffolds/internal/templates/config/kdefault/kustomization.go

Lines 79 to 84 in e116b54

    
           # Mount the controller config file for loading manager configurations 
        
           # through a ComponentConfig type 
        
           {{ if not .ComponentConfig }}#{{ end }}- manager_config_patch.yaml 
        
           # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in 
        
           # crd/kustomization.yaml

pkg/plugins/common/kustomize/v1/scaffolds/init.go

pkg/plugins/common/kustomize/v2-alpha/scaffolds/init.go

laxmikantbpandhare · 2022-07-21T18:42:40Z

Hi @camilamacedo86 I am trying to align my own project to incorporate these changes manually (since its already scaffolded). A clarification regarding the last comment which might be important for this PR too

We should ONLY add to the default kustomization the content which is relevant to the component config when the flag is used

Is the generatorOptions required when creating the project without the config flag. In other words, without the flag should the kustomization.yaml be
resources:
- manager.yaml
or
resources:
- manager.yaml

generatorOptions:
  disableNameSuffixHash: true
I am pretty sure that all of
configMapGenerator:
- name: manager-config
  files:
  - controller_manager_config.yaml
needs to be removed though.

Yes, you are correct.

We are removing the below completely.

configMapGenerator:
- name: manager-config
  files:
  - controller_manager_config.yaml

I will update the PR soon.

test/e2e/v3/generate_test.go

pkg/plugins/common/kustomize/v1/scaffolds/init.go

laxmikantbpandhare · 2022-08-05T09:12:54Z

@SayakMukhopadhyay @camilamacedo86 - Updated the PR with the new changes that we discussed for default/kustomization.yaml scaffolding. Could you please review once.

docs/book/src/component-config-tutorial/api-changes.md

SayakMukhopadhyay · 2022-08-05T09:29:32Z

@laxmikantbpandhare please correct me if I am wrong, from what I understand, if the ComponentConfig is disabled
instead of

# Mount the controller config file for loading manager configurations
# through a ComponentConfig type
#- manager_config_patch.yaml

the resultant section of the kustomization.yaml would be

# Mount the controller config file for loading manager configurations
# through a ComponentConfig type

docs/book/src/component-config-tutorial/api-changes.md

camilamacedo86

@laxmikantbpandhare very nice work.
Just a few nits and could you please squash the commits?

laxmikantbpandhare · 2022-08-05T09:30:53Z

@laxmikantbpandhare please correct me if I am wrong, from what I understand, if the ComponentConfig is disabled instead of
# Mount the controller config file for loading manager configurations
# through a ComponentConfig type
#- manager_config_patch.yaml
the resultant section of the kustomization.yaml would be
# Mount the controller config file for loading manager configurations
# through a ComponentConfig type

@SayakMukhopadhyay Yes, I will remove those comments as well.

laxmikantbpandhare · 2022-08-05T09:32:35Z

@laxmikantbpandhare very nice work. Just a few nits and could you please squash the commits?

Sure @camilamacedo86

docs/book/src/component-config-tutorial/api-changes.md

camilamacedo86

Great 🥇

/approved

In a follow up we need to create a plugin that does this config.
Then, we might also implement the edit subcommand to do the required changes

laxmikantbpandhare · 2022-08-05T14:04:39Z

/test pull-kubebuilder-e2e-k8s-1-19-16

laxmikantbpandhare · 2022-08-05T14:05:31Z

Great 🥇

/approved

In a follow up we need to create a plugin that does this config. Then, we might also implement the edit subcommand to do the required changes

Yes @camilamacedo86, I will work on those as well.

everettraven

Just a couple minor nits from me. Other than that it looks great! Great work @laxmikantbpandhare !

docs/book/src/component-config-tutorial/api-changes.md

pkg/plugins/common/kustomize/v1/scaffolds/init.go

pkg/plugins/common/kustomize/v2-alpha/scaffolds/init.go

updated testdata update v2-alpha and modified tests and added component-confit flag in it modified tests and enables componenet config flag removed unnecessary addition componenent config flag in v2 whenre this flag is not available removed for tests modified kustomization.yml file modified testdata added end to end tests with component config field marked as true line length is more than 122 characters mis spell updated doc modified docs modified docs modified default kustomization file too updated scaffold condition worked on review comments updated according to code review comments

everettraven

/lgtm

k8s-ci-robot · 2022-08-05T19:37:32Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

everettraven · 2022-08-05T19:38:20Z

Oops, I forgot that an LGTM on this repo auto merges.

@camilamacedo86 is it okay that this has merged?

laxmikantbpandhare · 2022-08-05T19:39:51Z

Oops, I forgot that an LGTM on this repo auto merges.

@camilamacedo86 is it okay that this has merged?

No worries, thanks for your comments.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 13, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 13, 2022

k8s-ci-robot requested review from joelanford and rashmigottipati July 13, 2022 23:13

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 Jul 13, 2022

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 Jul 14, 2022

camilamacedo86 reviewed Jul 19, 2022

View reviewed changes

pkg/plugins/common/kustomize/v1/scaffolds/init.go Show resolved Hide resolved

camilamacedo86 reviewed Jul 19, 2022

View reviewed changes

test/e2e/deployimage/plugin_cluster_test.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Jul 19, 2022

View reviewed changes

test/e2e/deployimage/plugin_cluster_test.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Jul 19, 2022

View reviewed changes

test/e2e/v4/generate_test.go Show resolved Hide resolved

laxmikantbpandhare changed the title ~~✨ ComponentConfig scaffolds should not be done by default~~ 🐛 ComponentConfig scaffolds should not be done by default Jul 19, 2022

laxmikantbpandhare changed the title ~~🐛 ComponentConfig scaffolds should not be done by default~~ 🐛 (kustomize/v1 and kustomize/v2-alpha) : ComponentConfig scaffolds should not be done by default Jul 19, 2022

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 19, 2022

SayakMukhopadhyay suggested changes Jul 21, 2022

View reviewed changes

pkg/plugins/common/kustomize/v1/scaffolds/init.go Outdated Show resolved Hide resolved

pkg/plugins/common/kustomize/v2-alpha/scaffolds/init.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Jul 21, 2022

View reviewed changes

test/e2e/v3/generate_test.go Show resolved Hide resolved

camilamacedo86 reviewed Jul 21, 2022

View reviewed changes

pkg/plugins/common/kustomize/v1/scaffolds/init.go Show resolved Hide resolved

camilamacedo86 reviewed Aug 5, 2022

View reviewed changes

docs/book/src/component-config-tutorial/api-changes.md Outdated Show resolved Hide resolved

camilamacedo86 reviewed Aug 5, 2022

View reviewed changes

docs/book/src/component-config-tutorial/api-changes.md Outdated Show resolved Hide resolved

camilamacedo86 reviewed Aug 5, 2022

View reviewed changes

docs/book/src/component-config-tutorial/api-changes.md Outdated Show resolved Hide resolved

laxmikantbpandhare requested a review from camilamacedo86 August 5, 2022 09:49

camilamacedo86 approved these changes Aug 5, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2022

camilamacedo86 requested review from everettraven and removed request for joelanford August 5, 2022 10:24

everettraven suggested changes Aug 5, 2022

View reviewed changes

docs/book/src/component-config-tutorial/api-changes.md Outdated Show resolved Hide resolved

pkg/plugins/common/kustomize/v1/scaffolds/init.go Outdated Show resolved Hide resolved

pkg/plugins/common/kustomize/v2-alpha/scaffolds/init.go Outdated Show resolved Hide resolved

laxmikantbpandhare requested a review from everettraven August 5, 2022 18:39

everettraven approved these changes Aug 5, 2022

View reviewed changes

k8s-ci-robot assigned everettraven Aug 5, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2022

k8s-ci-robot merged commit 20828c7 into kubernetes-sigs:master Aug 5, 2022

camilamacedo86 mentioned this pull request Sep 10, 2022

Meta - ComponentConfig scaffolds should not be done by default #2782

Closed

2 tasks

laxmikantbpandhare mentioned this pull request Sep 29, 2022

New changes in kubebuilder causing issue in SDK testcases operator-framework/operator-sdk#6046

Closed

This was referenced Oct 9, 2022

Component-config flag enabled causing issue #3009

Closed

🐛: fixed bug for component-config flag #3010

Merged

🐛 : leader-elect arg scaffolded should be scaffolded when component config option is not used #3013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 (kustomize/v1 and kustomize/v2-alpha) : ComponentConfig scaffolds should not be done by default #2826

🐛 (kustomize/v1 and kustomize/v2-alpha) : ComponentConfig scaffolds should not be done by default #2826

laxmikantbpandhare commented Jul 13, 2022 •

edited

Loading

k8s-ci-robot commented Jul 13, 2022

laxmikantbpandhare commented Jul 13, 2022

camilamacedo86 commented Jul 14, 2022

camilamacedo86 commented Jul 14, 2022

laxmikantbpandhare commented Jul 14, 2022

laxmikantbpandhare commented Jul 14, 2022 •

edited

Loading

laxmikantbpandhare commented Jul 19, 2022

camilamacedo86 commented Jul 20, 2022 •

edited

Loading

laxmikantbpandhare commented Jul 20, 2022 •

edited

Loading

SayakMukhopadhyay commented Jul 21, 2022 •

edited

Loading

SayakMukhopadhyay left a comment

laxmikantbpandhare commented Jul 21, 2022 •

edited

Loading

laxmikantbpandhare commented Aug 5, 2022 •

edited

Loading

SayakMukhopadhyay commented Aug 5, 2022 •

edited

Loading

camilamacedo86 left a comment

laxmikantbpandhare commented Aug 5, 2022

laxmikantbpandhare commented Aug 5, 2022

camilamacedo86 left a comment

laxmikantbpandhare commented Aug 5, 2022

laxmikantbpandhare commented Aug 5, 2022

everettraven left a comment

everettraven left a comment

k8s-ci-robot commented Aug 5, 2022

everettraven commented Aug 5, 2022

laxmikantbpandhare commented Aug 5, 2022

	# Mount the controller config file for loading manager configurations
	# through a ComponentConfig type
	{{ if not .ComponentConfig }}#{{ end }}- manager_config_patch.yaml

	# [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in
	# crd/kustomization.yaml

🐛 (kustomize/v1 and kustomize/v2-alpha) : ComponentConfig scaffolds should not be done by default #2826

🐛 (kustomize/v1 and kustomize/v2-alpha) : ComponentConfig scaffolds should not be done by default #2826

Conversation

laxmikantbpandhare commented Jul 13, 2022 • edited Loading

A description of the change

The motivation for the change

Tests done to ensure the changes:

k8s-ci-robot commented Jul 13, 2022

laxmikantbpandhare commented Jul 13, 2022

camilamacedo86 commented Jul 14, 2022

camilamacedo86 commented Jul 14, 2022

laxmikantbpandhare commented Jul 14, 2022

laxmikantbpandhare commented Jul 14, 2022 • edited Loading

laxmikantbpandhare commented Jul 19, 2022

camilamacedo86 commented Jul 20, 2022 • edited Loading

laxmikantbpandhare commented Jul 20, 2022 • edited Loading

SayakMukhopadhyay commented Jul 21, 2022 • edited Loading

SayakMukhopadhyay left a comment

Choose a reason for hiding this comment

laxmikantbpandhare commented Jul 21, 2022 • edited Loading

laxmikantbpandhare commented Aug 5, 2022 • edited Loading

SayakMukhopadhyay commented Aug 5, 2022 • edited Loading

camilamacedo86 left a comment

Choose a reason for hiding this comment

laxmikantbpandhare commented Aug 5, 2022

laxmikantbpandhare commented Aug 5, 2022

camilamacedo86 left a comment

Choose a reason for hiding this comment

laxmikantbpandhare commented Aug 5, 2022

laxmikantbpandhare commented Aug 5, 2022

everettraven left a comment

Choose a reason for hiding this comment

everettraven left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 5, 2022

everettraven commented Aug 5, 2022

laxmikantbpandhare commented Aug 5, 2022

laxmikantbpandhare commented Jul 13, 2022 •

edited

Loading

laxmikantbpandhare commented Jul 14, 2022 •

edited

Loading

camilamacedo86 commented Jul 20, 2022 •

edited

Loading

laxmikantbpandhare commented Jul 20, 2022 •

edited

Loading

SayakMukhopadhyay commented Jul 21, 2022 •

edited

Loading

laxmikantbpandhare commented Jul 21, 2022 •

edited

Loading

laxmikantbpandhare commented Aug 5, 2022 •

edited

Loading

SayakMukhopadhyay commented Aug 5, 2022 •

edited

Loading