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

Moves functionality from gitrepo to gitjob controller #2475

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

0xavi0
Copy link
Contributor

@0xavi0 0xavi0 commented May 31, 2024

Moves the following from gitrepo controller to gitjob controller:

  • AuthorizeAndAssignDefaults
  • RBAC resources creation
  • Helm secrets check
  • NewTargetsConfigMap

The gitrepo controller still purges bundles and bundledeployments on gitrepo deletion and handles the status coming from bundles and bundledeployments

Some functionality that was shared between both controllers has been moved to a common package to avoid code repetition.

Refers to: #2435

@0xavi0 0xavi0 added this to the v2.9.0 milestone May 31, 2024
@0xavi0 0xavi0 self-assigned this May 31, 2024
@0xavi0 0xavi0 requested a review from a team as a code owner May 31, 2024 07:45
@0xavi0 0xavi0 force-pushed the 2435-gitopts-gitrepo-controllers branch 5 times, most recently from 9f85170 to 84d478d Compare May 31, 2024 08:45
Copy link
Contributor

@weyfonk weyfonk left a comment

Choose a reason for hiding this comment

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

This almost LGTM, pending clarification on needed verbs for permissions on roles and role bindings. The rest is mostly nitpicks.
Thanks for this :)

charts/fleet/templates/rbac_gitjob.yaml Outdated Show resolved Hide resolved
integrationtests/gitjob/controller/controller_test.go Outdated Show resolved Hide resolved
integrationtests/gitjob/controller/controller_test.go Outdated Show resolved Hide resolved
integrationtests/gitjob/controller/controller_test.go Outdated Show resolved Hide resolved
@0xavi0 0xavi0 force-pushed the 2435-gitopts-gitrepo-controllers branch from 84d478d to 4a6411a Compare June 3, 2024 16:19
@0xavi0 0xavi0 force-pushed the 2435-gitopts-gitrepo-controllers branch 2 times, most recently from 6f0300e to ee3135c Compare June 4, 2024 13:59
@0xavi0 0xavi0 requested a review from weyfonk June 5, 2024 07:50
weyfonk
weyfonk previously approved these changes Jun 5, 2024
@@ -59,14 +62,18 @@ var _ = BeforeSuite(func() {
Expect(err).NotTo(HaveOccurred())
Expect(k8sClient).NotTo(BeNil())

mgr, err := ctrl.NewManager(cfg, ctrl.Options{
mgr, err := ctrlruntime.NewManager(cfg, ctrlruntime.Options{
Copy link
Member

Choose a reason for hiding this comment

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

nit: We use ctrl everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah, I forgot to change that back to ctrl.
There was already a variable around called ctrl and they were clashing.

It should be back to the standard ctrl now

Moves the following from `gitrepo` controller to `gitjob` controller:
* AuthorizeAndAssignDefaults
* RBAC resources creation
* Helm secrets check
* NewTargetsConfigMap

The `gitrepo` controller still purges `bundles` and `bundledeployments`
on `gitrepo` deletion and handles the status coming from `bundles` and
`bundledeployments`

Some functionality that was shared between both controllers has been
moved to a common package to avoid code repetition.

Signed-off-by: Xavi Garcia <[email protected]>
@0xavi0 0xavi0 force-pushed the 2435-gitopts-gitrepo-controllers branch from ee3135c to ac31217 Compare June 5, 2024 10:47
@0xavi0 0xavi0 requested review from manno and weyfonk June 5, 2024 10:53
@0xavi0 0xavi0 merged commit 753d23b into rancher:main Jun 5, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants