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

No GC Handler #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

No GC Handler #27

wants to merge 1 commit into from

Conversation

alecmerdler
Copy link
Contributor

Description

If an ImageManifestVuln is created by a user with the noop label, don't garbage collect it.

Fixes this test issue with operator-scorecard

@alecmerdler alecmerdler requested a review from kleesc December 17, 2019 23:14
Copy link
Member

@kleesc kleesc left a comment

Choose a reason for hiding this comment

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

LGTM

@alecmerdler alecmerdler force-pushed the noop-handler branch 2 times, most recently from fdb47d0 to 61fb857 Compare December 18, 2019 18:16
// Noop label
if _, ok := updatedManifest.GetLabels()["imagemanifestvuln.example"]; ok {
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Put this before updatedManifest is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -506,3 +511,12 @@ func (l *Labeller) podInNamespaces(pod *corev1.Pod) bool {
}
return false
}

func (l *Labeller) handleNoop(obj, oldObj *secscanv1alpha1.ImageManifestVuln) {
if _, ok := obj.GetLabels()["imagemanifestvuln.example"]; ok && !reflect.DeepEqual(obj.Spec, oldObj.Spec) {
Copy link
Member

@kleesc kleesc Dec 18, 2019

Choose a reason for hiding this comment

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

Should we change the label name to something more descriptive?
imagemanifestvuln.skipgc?
Also, is an annotation better for this use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


func (l *Labeller) handleNoop(obj, oldObj *secscanv1alpha1.ImageManifestVuln) {
if _, ok := obj.GetLabels()["imagemanifestvuln.example"]; ok && !reflect.DeepEqual(obj.Spec, oldObj.Spec) {
_, err := l.sclient.SecscanV1alpha1().ImageManifestVulns(obj.GetNamespace()).UpdateStatus(updateImageManifestVulnLastUpdate(obj))
Copy link
Member

Choose a reason for hiding this comment

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

It's not really a noop if it's updating the status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, noop isn't the best wording. We're essentially trying to implement something that will pass the operator-scorecard test that assumes that a user will create an ImageManifestVuln from the alm-examples annotation on our CSV, and expects the status block to change if they update the spec. This Operator doesn't work like that, but we can add a special handler for it.

@alecmerdler alecmerdler force-pushed the noop-handler branch 2 times, most recently from 649685d to f208adf Compare December 18, 2019 23:25
@alecmerdler alecmerdler changed the title Noop Handler No GC Handler Dec 19, 2019
Copy link
Member

@kleesc kleesc left a comment

Choose a reason for hiding this comment

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

LGTM

@alecmerdler
Copy link
Contributor Author

Holding off on merging until I confirm it passes the operator-scorecard tests.

@openshift-ci
Copy link

openshift-ci bot commented Mar 27, 2022

@alecmerdler: PR needs rebase.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants