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

✨deploy-image/v1-alpha: add finalizers for the Custom Resource created #2793

Merged

Conversation

NikhilSharmaWe
Copy link
Member

@NikhilSharmaWe NikhilSharmaWe commented Jul 3, 2022

Description

Add finalizers for the Custom Resource created for deleting the deployment under before deleting the CR.

Motivation

Fixes part of #2765

@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 Jul 3, 2022
@NikhilSharmaWe NikhilSharmaWe force-pushed the finalizersDeployImage branch 2 times, most recently from 2f48f9d to 96080da Compare July 4, 2022 08:25
@NikhilSharmaWe NikhilSharmaWe force-pushed the finalizersDeployImage branch 2 times, most recently from 1ac6046 to 86edad5 Compare July 5, 2022 10:02
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.

/hold

Until the fixes be made

@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 Jul 5, 2022
}, time.Minute, time.Second).Should(Succeed())

EventuallyWithOffset(1, func() error {
events, err := kbc.Kubectl.Get(true, "events", "--field-selector", "type", "Warning",
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
events, err := kbc.Kubectl.Get(true, "events", "--field-selector", "type", "Warning",
events, err := kbc.Kubectl.Get(true, "events", "--field-selector=type=Warning",

TO fix the command:
Screenshot 2022-07-05 at 23 03 18

"time"

"github.com/go-logr/logr"
Copy link
Member

@camilamacedo86 camilamacedo86 Jul 5, 2022

Choose a reason for hiding this comment

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

Suggested change
"github.com/go-logr/logr"

@NikhilSharmaWe we need to use the log from the controller-runtime which is provided in the reconcile (see the log var) and not this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was using that log only but I was using this for specifying the parameters of the finalize function but since now that parameter is not needed(due some changes in the finalize function), I am removing it.

@NikhilSharmaWe NikhilSharmaWe force-pushed the finalizersDeployImage branch from 86edad5 to e57c24e Compare July 6, 2022 09:46
@NikhilSharmaWe NikhilSharmaWe force-pushed the finalizersDeployImage branch from e57c24e to 748178a Compare July 6, 2022 09:55
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2022
@NikhilSharmaWe NikhilSharmaWe force-pushed the finalizersDeployImage branch from 8b630f8 to 748178a Compare July 6, 2022 10:41
@NikhilSharmaWe NikhilSharmaWe force-pushed the finalizersDeployImage branch 2 times, most recently from 1c40bd1 to 069ffa6 Compare July 6, 2022 11:57
@@ -66,27 +66,33 @@ package {{ if and .MultiGroup .Resource.Group }}{{ .Resource.PackageName }}{{ el
import (
Copy link
Member

Choose a reason for hiding this comment

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

@NikhilSharmaWe,

The new occurs because the recorder is empty and needs to be passed when we create the controller and pass it in the manager ( main.go ):

if err = (&controllers.FoorxobReconciler{
   Client:   mgr.GetClient(),
   Scheme:   mgr.GetScheme(),
   Recorder: mgr.GetEventRecorderFor("mykind-controller"),
}).SetupWithManager(mgr); err != nil {
   setupLog.Error(err, "unable to create controller", "controller", "Foorxob")
   os.Exit(1)
}
//+kubebuilder:scaffold:builder

Also, in the controller, it needs to be changed to be Upper case: Recorder record.EventRecorder
Then, when we call the event use Eventf instead of Event since we want to format the message

See that this PR does not have the e2e check as well. It is missing can you come back that?

@NikhilSharmaWe NikhilSharmaWe force-pushed the finalizersDeployImage branch 2 times, most recently from 069ffa6 to f604a6c Compare July 7, 2022 08:17
Comment on lines 127 to 137
func (s *apiScaffolder) updateMainEventRecorder() error {
defaultMainPath := "main.go"
err := util.InsertCode(defaultMainPath, `Scheme: mgr.GetScheme(),
`,
fmt.Sprintf(recorderTemplate, strings.ToLower(s.resource.Kind)))
if err != nil {
return fmt.Errorf("error scaffolding event recorder while creating the controller in main.go: %v", err)
}
return nil
}
Copy link
Member

@camilamacedo86 camilamacedo86 Jul 7, 2022

Choose a reason for hiding this comment

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

@NikhilSharmaWe,

Instead of use replace the best approach here would we create a MainUpdater like:

https://github.com/NikhilSharmaWe/kubebuilder/blob/f604a6cb2c4a3ba5425471ace96125630c9a50f3/pkg/plugins/golang/v3/scaffolds/api.go#L106-L110

Then, in this updater we will have another template for the controller in the main: https://github.com/NikhilSharmaWe/kubebuilder/blob/f604a6cb2c4a3ba5425471ace96125630c9a50f3/pkg/plugins/golang/v3/scaffolds/internal/templates/main.go#L100-L106

Could you please add a TODO on top of the method at least such as

// TODO: replace this implementation by creating its own MainUpdater which will have its own controller template which set the recorder.

Also, could you please change the target so we ensure that you ONLY set the event.Recorder on the Controller code of the specific api instead of adding the code in all of them? If we have a project with an API that uses the plugin and another API that does not use the plugin only the API that uses the plugin should have this addition.

@NikhilSharmaWe NikhilSharmaWe force-pushed the finalizersDeployImage branch from f604a6c to 2bc1a66 Compare July 8, 2022 03:20
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 8, 2022

@NikhilSharmaWe: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubebuilder-e2e-k8s-1-19-16 2bc1a66 link false /test pull-kubebuilder-e2e-k8s-1-19-16

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

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.

@NikhilSharmaWe,

The fix: https://github.com/kubernetes-sigs/kubebuilder/pull/2793/files#r915313169 is not a blocker for the PR. So, let's move forward and improve it in a follow up

/lgtm
/approved

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, NikhilSharmaWe

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 Jul 8, 2022
@camilamacedo86
Copy link
Member

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

@camilamacedo86
Copy link
Member

/hold cancel

@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 Jul 8, 2022
@k8s-ci-robot k8s-ci-robot merged commit 7b80206 into kubernetes-sigs:master Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants