Skip to content

Ensure that required is always fixed up by ApplySecret#1272

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
jerpeter1:fix_ensure_node_kubeconfig_test
Dec 17, 2021
Merged

Ensure that required is always fixed up by ApplySecret#1272
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
jerpeter1:fix_ensure_node_kubeconfig_test

Conversation

@jerpeter1
Copy link
Copy Markdown
Member

This is to fix TestEnsureNodeKubeconfigs in cluster-kube-apiserver's unit tests (I'm not sure if the test being broken is actually important or not, but we can re-address that later)

@jerpeter1
Copy link
Copy Markdown
Member Author

/assign @deads2k


// ApplySecret merges objectmeta, requires data
func ApplySecretImproved(ctx context.Context, client coreclientv1.SecretsGetter, recorder events.Recorder, requiredInput *corev1.Secret, cache ResourceCache) (*corev1.Secret, bool, error) {
existing, err := client.Secrets(requiredInput.Namespace).Get(ctx, requiredInput.Name, metav1.GetOptions{})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

existing, err := 
if err != nil && !apierrors.IsNotFound(err){
    return nil, false, err
}
if cache.SafeToSkipApply(requiredInput, existing) {
	return existing, false, nil
}

// do the required deep copy and string data stuff

if apierrors.IsNotFound(err) {
		requiredCopy := requiredInput.DeepCopy()
		actual, err := client.Secrets(requiredCopy.Namespace).
			Create(ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*corev1.Secret), metav1.CreateOptions{})
		reportCreateEvent(recorder, requiredCopy, err)
		cache.UpdateCachedResourceMetadata(requiredInput, actual)
		return actual, true, err
	}

// make sure all UpdateCachedResourceMetadata use requiredInput and not required

and you keep your "skip deep copy" behavior, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I just tested your suggestion, but I had to make a small change to get the unit tests to pass (requiredCopy := required.DeepCopy() instead of requiredCopy := requiredInput.DeepCopy()

@jerpeter1 jerpeter1 force-pushed the fix_ensure_node_kubeconfig_test branch from 20d4bcd to 0c35def Compare December 16, 2021 19:30
@jerpeter1 jerpeter1 changed the title Move check for existing Secret until after required is fixed up Ensure that required is always fixed up in by ApplySecret Dec 17, 2021
@jerpeter1 jerpeter1 changed the title Ensure that required is always fixed up in by ApplySecret Ensure that required is always fixed up by ApplySecret Dec 17, 2021
@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Dec 17, 2021

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 17, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, jerpeter1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 17, 2021

@jerpeter1: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-robot openshift-merge-robot merged commit d48a1fb into openshift:master Dec 17, 2021
eggfoobar added a commit to eggfoobar/cluster-storage-operator that referenced this pull request Dec 17, 2021
pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-csi-snapshot-controller-operator that referenced this pull request Dec 17, 2021
pull in to grab lfix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cloud-credential-operator that referenced this pull request Dec 17, 2021
pull in to grab lfix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-openshift-controller-manager-operator that referenced this pull request Dec 17, 2021
pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-authentication-operator that referenced this pull request Dec 17, 2021
pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-openshift-apiserver-operator that referenced this pull request Dec 17, 2021
pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-config-operator that referenced this pull request Dec 17, 2021
pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-image-registry-operator that referenced this pull request Dec 17, 2021
pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/console-operator that referenced this pull request Dec 17, 2021
pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
jerpeter1 added a commit to jerpeter1/cluster-kube-apiserver-operator that referenced this pull request Dec 17, 2021
jerpeter1 added a commit to jerpeter1/cluster-kube-scheduler-operator that referenced this pull request Dec 17, 2021
jerpeter1 added a commit to jerpeter1/cluster-kube-scheduler-operator that referenced this pull request Dec 17, 2021
jerpeter1 added a commit to jerpeter1/cluster-kube-apiserver-operator that referenced this pull request Dec 17, 2021
eggfoobar added a commit to eggfoobar/cluster-image-registry-operator that referenced this pull request Dec 24, 2021
pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-image-registry-operator that referenced this pull request Dec 24, 2021
pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>

bump: library-go

updated library-go to latest for fix in pr openshift/library-go#1273

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-csi-snapshot-controller-operator that referenced this pull request Jan 6, 2022
pull in to grab lfix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-csi-snapshot-controller-operator that referenced this pull request Jan 6, 2022
updated method scalls with new signatures
removed passing next generation to ApplyValidationWebhookConfiguration
updated to use explicit call to bind log flags to cli commandline

Signed-off-by: ehila <ehila@redhat.com>

upkeep: removed unused code and small clean up

Signed-off-by: ehila <ehila@redhat.com>

fix: pull in latest library-go

pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>

bump: library-go

updated library-go to latest for fix in pr openshift/library-go#1273

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-image-registry-operator that referenced this pull request Jan 7, 2022
pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>

bump: library-go

updated library-go to latest for fix in pr openshift/library-go#1273

Signed-off-by: ehila <ehila@redhat.com>
eggfoobar added a commit to eggfoobar/cluster-image-registry-operator that referenced this pull request Jan 25, 2022
fetched latest library-go and vendored dep
updated handlers to use updated sctruct names
updated okd builder image to use go 1.17

Signed-off-by: ehila <ehila@redhat.com>

fix: updated build-machinery to fix yaml-patch verify

updated build-machinery
- more info: openshift/build-machinery-go#58

Signed-off-by: ehila <ehila@redhat.com>

fix: pull in latest library-go

pull in to grab fix described here openshift/library-go#1272

Signed-off-by: ehila <ehila@redhat.com>

bump: library-go

updated library-go to latest for fix in pr openshift/library-go#1273

Signed-off-by: ehila <ehila@redhat.com>
dgrisonnet pushed a commit to dgrisonnet/cluster-kube-apiserver-operator that referenced this pull request Apr 26, 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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants