Skip to content

Conversation

@jhadvig
Copy link
Member

@jhadvig jhadvig commented Jan 2, 2018

This fix is based from https://bugzilla.redhat.com/show_bug.cgi?id=1529467 where we suggested that best fix would be just warn user that the envVar is invalid.
@jwforres @spadgett PTAL

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 2, 2018
<div class="help-block">
<span class="pf-icon pficon-warning-triangle-o" aria-hidden="true"></span>
Some of the keys for {{ctrl.apiObject.kind | humanizeKind}} <strong>{{ctrl.apiObject.metadata.name}}</strong> are not valid environment variable names and will not be added.
Some of the keys for {{ctrl.apiObject.kind | humanizeKind}} <strong>{{ctrl.apiObject.metadata.name}}</strong> are not valid environment variable names.
Copy link
Member

Choose a reason for hiding this comment

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

The message was saying just the invalid keys won't be added, not that the secret won't be added. But maybe the wording was confusing.

I think we still need to tell the user what will happen. How about...?

Some of the keys for {{ctrl.apiObject.kind | humanizeKind}} <strong>{{ctrl.apiObject.metadata.name}}</strong> are not valid environment variable names.
Only {{ctrl.apiObject.kind | humanizeKind}} keys with valid names will be added as environment variables.

cc @jwforres

Copy link
Member Author

Choose a reason for hiding this comment

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

that sounds good to me 👍

Copy link
Member

@jwforres jwforres Jan 2, 2018

Choose a reason for hiding this comment

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

So I like @spadgett's message better generally than what is there. My point in the bug is that we say "Some" even when its really "All"

Copy link
Member

Choose a reason for hiding this comment

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

If we say

{{ctrl.apiObject.kind | humanizeKind}} <strong>{{ctrl.apiObject.metadata.name}}</strong> contains keys that are not valid environment variable names.
Only {{ctrl.apiObject.kind | humanizeKind}} keys with valid names will be added as environment variables.

then one message works generally

Copy link
Member

Choose a reason for hiding this comment

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

will need to update that first humanized string so the first letter is capitalized, i.e. "Config map"

Copy link
Member

Choose a reason for hiding this comment

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

That message is better 👍

Will need upperFirst to capitalize the first word in the sentence.

{{ctrl.apiObject.kind | humanizeKind | upperFirst}} <strong>{{ctrl.apiObject.metadata.name}}</strong> ...

Copy link
Member

Choose a reason for hiding this comment

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

IIRC the capitalize option on humanizeKind filter capitalizes both words? if so then you can just wrap this humanizeKind filter call in parens and pass it to _.capitalize

Copy link
Member

Choose a reason for hiding this comment

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

@jwforres We were making the same comment at the same time :)

I think upperFirst filter should do what we want.

@jhadvig
Copy link
Member Author

jhadvig commented Jan 2, 2018

@spadgett @jwforres comments addressed

@spadgett
Copy link
Member

/lgtm

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

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit c9b1132 into openshift:master Jan 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants