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/v3-alpha) Adding support for scaffolding with a versioned ComponentConfig #1790

Merged
merged 2 commits into from
Nov 18, 2020
Merged

⚠ (go/v3-alpha) Adding support for scaffolding with a versioned ComponentConfig #1790

merged 2 commits into from
Nov 18, 2020

Conversation

christopherhein
Copy link
Member

@christopherhein christopherhein commented Nov 5, 2020 •
edited
Loading

This adds the ability to scaffold a kubebuilder v3 plugin project with a versioned config file (componentconfig) this was first implemented in Controller Runtime by allowing .AndFrom[OrDie]() to the ctrl.Manager examples can be found https://github.com/kubernetes-sigs/controller-runtime/blob/master/examples/configfile/builtin/main.go#L47-L49 with a config like https://github.com/kubernetes-sigs/controller-runtime/blob/master/examples/configfile/builtin/config.yaml I've first implemented this as a flag on init --componentconfig which is a boolean type. As developers use this the idea is they can implement custom configs with these standard configs embedded https://github.com/kubernetes-sigs/controller-runtime/blob/master/examples/configfile/custom/main.go#L49-L59 these files are decoded and made available making it easy to extend your controller config without relying on CLI flags.

For the kubebuilder implementation this scaffolds the config file and manager patch for volumes and configures kustomize to create a configmap for this config file.

I'm submitting this cause I'd like to get feedback before I go and refactor some of the fields to Mixins and Injection. As well this is blocked by #1746 for a stable controller-runtime 0.7.0 release and #1756 for v3 e2e tests.

Fixes #722

Signed-off-by: Chris Hein me@chrishein.com

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 5, 2020
@christopherhein christopherhein changed the title ⚠ WIP Adding support for scaffolding a with a versioned ComponentConfig ⚠ WIP Adding support for scaffolding with a versioned ComponentConfig Nov 5, 2020
@christopherhein christopherhein force-pushed the feature/component-config-implementation branch from 1ec2d75 to 53e280c Compare November 5, 2020 05:57
@Adirio
Copy link
Contributor

Adirio commented Nov 5, 2020

Prow is not detecting it as a WIP so I will manually hold it, feel free to remove it when its done
/hold wip

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 5, 2020
Adirio
Adirio suggested changes Nov 5, 2020
View reviewed changes
Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

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

Several comments:

pkg/plugin/v3/scaffolds/internal/templates/main.go Outdated Show resolved Hide resolved
pkg/plugin/v3/init.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/init.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/init.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/kdefault/auth_proxy_patch.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/manager/config.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/manager/kustomization.go Outdated Show resolved Hide resolved
pkg/plugin/v3/scaffolds/init.go Outdated Show resolved Hide resolved
@christopherhein christopherhein mentioned this pull request Nov 6, 2020
Merged
@christopherhein christopherhein force-pushed the feature/component-config-implementation branch 3 times, most recently from 28c55cf to 23e49e0 Compare November 7, 2020 08:01
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2020
@christopherhein christopherhein force-pushed the feature/component-config-implementation branch 2 times, most recently from ac37786 to 63e5efd Compare November 7, 2020 19:27
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2020
Adirio
Adirio reviewed Nov 7, 2020
View reviewed changes
Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

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

There are a lot of changes in the testdata folder that I don't think that are expected, could you check them?

@christopherhein
Copy link
Member Author

There are a lot of changes in the testdata folder that I don't think that are expected, could you check them?

Yea, the tests are failing cause of the 0.7.0 which I think we need to get the declarative addons package updated so all the files are generated properly.

@christopherhein christopherhein force-pushed the feature/component-config-implementation branch from 63e5efd to 433bc92 Compare November 8, 2020 04:01
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 8, 2020
@christopherhein
Copy link
Member Author

BTW Thanks @Adirio for all the reviews on this and the response time is incredibly fast 👏 at this point we're blocked on kubernetes-sigs/kubebuilder-declarative-pattern#127

@estroz
Copy link
Contributor

estroz commented Nov 9, 2020

@christopherhein #1804 bumps controller-runtime to v0.7.0-alpha.6

@christopherhein
Copy link
Member Author

Thanks @estroz

estroz
estroz approved these changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@estroz estroz 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 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 Nov 18, 2020
@Adirio
Copy link
Contributor

Adirio commented Nov 18, 2020

/retest

estroz
estroz suggested changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

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

PR needs a rebase and make generate

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2020
estroz
estroz reviewed Nov 18, 2020
View reviewed changes
generate_testdata.sh Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2020
@christopherhein christopherhein force-pushed the feature/component-config-implementation branch from 7f852e3 to defa6a4 Compare November 18, 2020 21:22
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and 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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 18, 2020
@christopherhein christopherhein force-pushed the feature/component-config-implementation branch from defa6a4 to b81fd49 Compare November 18, 2020 21:31
@christopherhein
adding support for scaffolding componentconfig type
a33c027 
Signed-off-by: Chris Hein <me@chrishein.com>
@christopherhein
regenerating the testdata
55b8fd7 
Signed-off-by: Chris Hein <me@chrishein.com>
@christopherhein christopherhein force-pushed the feature/component-config-implementation branch from b81fd49 to 55b8fd7 Compare November 18, 2020 22:51
estroz
estroz approved these changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@estroz estroz 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 Nov 18, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, christopherhein, estroz

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2020
@k8s-ci-robot k8s-ci-robot merged commit 4e1828b into kubernetes-sigs:master Nov 18, 2020
@christopherhein
Copy link
Member Author

🎉 thanks @Adirio @estroz @camilamacedo86 really appreciate all the help getting this through.

@christopherhein christopherhein deleted the feature/component-config-implementation branch November 18, 2020 23:19
@luxas
Copy link

luxas commented Nov 20, 2020

Kudos to everyone, amazing to see this land 🎉 👏 🥇 !!

@camilamacedo86 camilamacedo86 mentioned this pull request Feb 18, 2021
Closed
@Adirio Adirio mentioned this pull request Mar 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Adirio Adirio Adirio approved these changes

@camilamacedo86 camilamacedo86 camilamacedo86 left review comments

@estroz estroz estroz approved these changes

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

Assignees

@Adirio Adirio

@estroz estroz

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement ComponentConfig by default & stop using (most) flags
6 participants
@christopherhein @Adirio @estroz @k8s-ci-robot @luxas @camilamacedo86