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

Fix: Preserve secret if rootPasswordSecretKeyRef is unset (#1148) #1168

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

Conversation

AlPepino
Copy link

@AlPepino AlPepino commented Feb 17, 2025

Fixes #1148

@mmontes11 mmontes11 self-requested a review February 18, 2025 09:16
Copy link
Member

@mmontes11 mmontes11 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution @AlPepino!

I think you are going in the right direction, but some adaptations are needed. Could you take a look at my comments?

}
return secret, nil
}
package builder
Copy link
Member

Choose a reason for hiding this comment

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

The changes displayed are a bit odd, why the whole file has been marked as replaced? The owner changes LGTM, but can we make only the changes needed? This will help building context based on history in the future.

Copy link
Member

Choose a reason for hiding this comment

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

This is making the linting fail. Can we fix it?

}
assert.Equal(t, tt.opts.Data, secret.Data)
assert.Equal(t, tt.opts.Key.Name, secret.Name)
assert.Equal(t, tt.opts.Key.Namespace, secret.Namespace)
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an assertion here to verify that the secret has a controller reference? Basiscally, when tt.owner is set, we need to verify that the controllerutil.HasControllerReference is true. See:
https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/controller/controllerutil#HasControllerReference

if err != nil {
t.Fatalf("unexpected error creating discovery: %v", err)
}
builder := newTestBuilder(discovery)
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
builder := newTestBuilder(discovery)
builder := newDefaultTestBuilder(t)

This way you don't need to instanciate a discovery and the tests will run

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

Successfully merging this pull request may close these issues.

[Bug] operator deletes auto-generated root password
2 participants