-
Notifications
You must be signed in to change notification settings - Fork 251
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
Update how ci-operator gets cluster profile secrets #4112
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: psalajova 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 |
/retest |
cmd/ci-operator/main.go
Outdated
// Use config-resolver to get details about the cluster profile (which includes the secret's name) | ||
cpDetails, err := resolverClient.ClusterProfile(clusterProfile) | ||
if err != nil { | ||
logrus.WithError(err).Errorf("failed to retrieve details from config resolver for '%s' cluster profile", clusterProfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to log the error, since you are returning it.
cmd/ci-operator/main.go
Outdated
} | ||
|
||
// getSecretFromCiNamespace retrieves a secret from the ci namespace | ||
func getSecretFromCiNamespace(secretName string, client ctrlruntimeclient.Client, ctx context.Context) (*coreapi.Secret, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are using this function only once, and it is a few lines of code you can do it directly. Also, the unit tests you added for this function are not testing anything valuable, you can remove them.
/test remaining-required |
/test e2e |
/test e2e |
1 similar comment
/test e2e |
/test e2e |
1 similar comment
/test e2e |
/test e2e |
5040fe8
to
4cd725c
Compare
/test e2e |
71ae618
to
c5487ca
Compare
/test e2e |
} | ||
|
||
// getClusterProfileNamesFromTargets extracts the needed cluster profile name(s) from the target arg(s) | ||
func (o *options) getClusterProfileNamesFromTargets() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use unittests for this.
cmd/ci-operator/main_test.go
Outdated
return nil, nil | ||
} | ||
|
||
func TestGetClusterProfileSecret(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically you are not testing anything. The main operation that is being done to retrieve the cluster profiles is being tested in https://github.com/psalajova/ci-tools/blob/35c5f8f232ef977adf77ce20c237560df3e4eea8/pkg/registry/server/client_test.go#L88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, right
// getClusterProfileNamesFromTargets extracts the needed cluster profile name(s) from the target arg(s) | ||
func (o *options) getClusterProfileNamesFromTargets() { | ||
for _, targetName := range o.targets.values { | ||
for _, test := range o.configSpec.Tests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider having a helper method on the Tests object to retrieve the cluster profiles.
c5487ca
to
363f7c6
Compare
/test e2e |
/label tide/merge-method-squash |
/test e2e |
@psalajova: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
/hold to synchronize with rolling out of the other changes that need to be done |
Use config-resolver to get the name of cluster profile secret we need, instead of mounting it to the job and then reading it locally along with the other user-defined secrets.
Next steps:
/cc @droslean