Skip to content

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Feb 4, 2018

This patch makes a tiny change by adding source secret name validation
to new build.

Currently even though build-secret has the validation, source secret
does not.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 4, 2018
@mfojtik
Copy link
Contributor

mfojtik commented Feb 5, 2018

@nak3 can you add quick unit test that exercise this with wrong secret name and correct secret name?

@nak3
Copy link
Contributor Author

nak3 commented Feb 5, 2018

@mfojtik Thank you. I added the test to test/cmd/newapp.sh, but if you would like me to add unit test code, I will add it to newapp_test.go(?) so please let me know. (I am wondering that test/cmd/newapp.sh would be fine).

@@ -898,6 +898,9 @@ func (c *AppConfig) Run() (*AppResult, error) {
}
}
if len(c.SourceSecret) > 0 {
if len(validation.ValidateSecretName(c.SourceSecret, false)) != 0 {
return nil, fmt.Errorf("the %q must be valid secret name", c.SourceSecret)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest "source secret name %q is invalid"

@jim-minter
Copy link
Contributor

@nak3 thanks for the PR. The test looks fine, please just update the actual error text and I'll lgtm :)

@nak3
Copy link
Contributor Author

nak3 commented Feb 6, 2018

@jim-minter Thank you. Sure, I updated.

@jim-minter
Copy link
Contributor

Thanks @nak - one more thing - please squash the commits.

This patch makes a tiny change by adding source secret name validation
to new build.

Currently even though build-secret has the validation, source secret
does not.
@nak3
Copy link
Contributor Author

nak3 commented Feb 7, 2018

@jim-minter Thank you. Sure, I have done now.

@jim-minter
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2018
@bparees
Copy link
Contributor

bparees commented Feb 7, 2018

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, jim-minter, nak3

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2018
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 05158a7 into openshift:master Feb 7, 2018
@nak3 nak3 deleted the source-secret-validation branch February 7, 2018 07:24
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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants