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.

Sign up for GitHub

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

Jump to bottom

make templateinstance secret optional #14848

Merged
merged 1 commit into from
Jun 29, 2017
Merged

make templateinstance secret optional #14848

merged 1 commit into from
Jun 29, 2017

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Jun 23, 2017

fixes #14837

@bparees bparees force-pushed the optional_secret branch 4 times, most recently from 8d0a629 to 753bbcc Compare June 23, 2017 03:33
@bparees
Copy link
Contributor Author

bparees commented Jun 23, 2017

[test]

@bparees
Copy link
Contributor Author

bparees commented Jun 23, 2017

[testextended][extended:core(templates)]

@bparees
Copy link
Contributor Author

bparees commented Jun 23, 2017

@jim-minter ptal

i debated doing the same w/ brokertemplateinstance but there were more changes involved and it seemed more reasonable to assume there would be parameters always coming on a provision call, so for now at least i left that alone.

@bparees bparees force-pushed the optional_secret branch 2 times, most recently from 7e9884e to 8439ca8 Compare June 23, 2017 04:17
jim-minter
jim-minter reviewed Jun 23, 2017
View reviewed changes
pkg/template/api/validation/validation.go Outdated
@@ -61,7 +61,7 @@ func ValidateTemplateInstance(templateInstance *api.TemplateInstance) (allErrs f
err.Field = "spec.template." + err.Field
allErrs = append(allErrs, err)
}
if templateInstance.Spec.Secret.Name != "" {
if templateInstance.Spec.Secret != nil && templateInstance.Spec.Secret.Name != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

think you need to return a field.Required if Spec.Secret != nil && Spec.Secret.Name == ""

Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow, this logic was completely broken before, I'm glad you submitted this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update, please check my work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I was thinking:

	if templateInstance.Spec.Secret != nil {
		if templateInstance.Spec.Secret.Name == "" {
			allErrs = append(allErrs, field.Required(field.NewPath("spec.secret.name"), ""))
		} else {
			for _, msg := range oapi.GetNameValidationFunc(validation.ValidateSecretName)(templateInstance.Spec.Secret.Name, false) {
				allErrs = append(allErrs, field.Invalid(field.NewPath("spec.secret.name"), templateInstance.Spec.Secret.Name, msg))
			}
		}
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

please ignore comment above, was looking at old version of the PR - all fine

jim-minter
jim-minter reviewed Jun 23, 2017
View reviewed changes
pkg/template/api/validation/validation_test.go Outdated
@@ -367,7 +367,7 @@ func TestValidateTemplateInstanceUpdate(t *testing.T) {
},
},
},
Secret: kapi.LocalObjectReference{
Secret: &kapi.LocalObjectReference{
Copy link
Contributor

Choose a reason for hiding this comment

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

might as well add one more TC that proves validation passes when Secret is nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i could have sworn i did, not sure what happened to it. will add it again :)

jim-minter
jim-minter reviewed Jun 23, 2017
View reviewed changes
pkg/template/controller/templateinstance_controller.go
secret, err := c.kc.Core().Secrets(templateInstance.Namespace).Get(templateInstance.Spec.Secret.Name, metav1.GetOptions{})
if err != nil {
return err
s, err := c.kc.Core().Secrets(templateInstance.Namespace).Get(templateInstance.Spec.Secret.Name, metav1.GetOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

just condenses down to secret, err = c.kc...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, because that shadows the secret var defined above (which must be defined above because it needs to exist outside this scope). and i can't use "=" because err is actually a new variable.

unless i'm missing a trick, this is the joys of golang combining implicit variable declaration, multivariable returns, and zero shadowing protection.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, ok - missed that the err := above was in an if scope.

@jim-minter
Copy link
Contributor

Bar nits looks good, many thanks!

i debated doing the same w/ brokertemplateinstance but there were more changes involved and it seemed more reasonable to assume there would be parameters always coming on a provision call, so for now at least i left that alone.

Agreed.

Can I ask one more favour as part of this PR - can you remove lines 70-78 of templateinstance_impersonation.go (// post the status to avoid kicking off the controller) as they are nonsense?

@bparees
Copy link
Contributor Author

bparees commented Jun 23, 2017

@jim-minter comments addressed in new commit, other than the shadowing bit. if i've missed a trick there, let me know.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2017
bparees
bparees commented Jun 24, 2017
View reviewed changes
test/extended/templates/templateinstance_impersonation.go
Status: kapi.ConditionTrue,
},
},
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jim-minter please confirm this is the bit you wanted removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, thanks

@openshift openshift deleted a comment from openshift-bot Jun 24, 2017
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2017
@openshift openshift deleted a comment from openshift-bot Jun 24, 2017
@openshift openshift deleted a comment from openshift-bot Jun 24, 2017
@openshift openshift deleted a comment from openshift-bot Jun 25, 2017
@openshift openshift deleted a comment from openshift-bot Jun 25, 2017
@openshift openshift deleted a comment from openshift-bot Jun 25, 2017
@openshift openshift deleted a comment from openshift-bot Jun 25, 2017
@jim-minter
Copy link
Contributor

lgtm, thanks!

@bparees
Copy link
Contributor Author

bparees commented Jun 26, 2017

@openshift/api-review can we get a quick sign off on this (changing secretref field to a pointer to make it optional)

@smarterclayton
Copy link
Contributor

approved

@bparees
Copy link
Contributor Author

bparees commented Jun 26, 2017

[merge]

@deads2k
Copy link
Contributor

deads2k commented Jun 26, 2017

re[test]

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2017
@bparees bparees force-pushed the optional_secret branch 4 times, most recently from 873d3c1 to 6a8081d Compare June 26, 2017 23:20
@openshift openshift deleted a comment from openshift-bot Jun 26, 2017
@openshift openshift deleted a comment from openshift-bot Jun 26, 2017
@openshift openshift deleted a comment from openshift-bot Jun 26, 2017
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2017
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 073e82c

@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to 073e82c

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/755/) (Base Commit: 247631a) (PR Branch Commit: 073e82c) (Extended Tests: core(templates))

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2675/) (Base Commit: 247631a) (PR Branch Commit: 073e82c)

@bparees
Copy link
Contributor Author

bparees commented Jun 29, 2017

[merge]

@bparees
Copy link
Contributor Author

bparees commented Jun 29, 2017 via email

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 073e82c

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 29, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1174/) (Base Commit: 73cdfc3) (PR Branch Commit: 073e82c) (Image: devenv-rhel7_6411)

@openshift-bot openshift-bot merged commit f4b675e into openshift:master Jun 29, 2017
@bparees bparees deleted the optional_secret branch June 30, 2017 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jim-minter jim-minter jim-minter left review comments

Assignees
No one assigned
Labels
api-approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make secret ref optional in templateinstance
5 participants
@bparees @jim-minter @smarterclayton @deads2k @openshift-bot