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

Bug 1509142 - Should not display the 'Reveal Secret' link when secret is without 'data' field #2448

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

jhadvig
Copy link
Member

@jhadvig jhadvig commented Nov 3, 2017

@spadgett PTAL

@jhadvig jhadvig requested a review from spadgett November 3, 2017 12:59
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 3, 2017
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

We should disable the "Add to Application" button if there is not data.

Can you check "Environment From" when you pick the secret? I suspect the view details dialog has the same problem.

@@ -90,6 +90,9 @@ <h2 class="mar-top-none">
</div>
</div>
</dl>
<p ng-if="!secret.data" class="mar-bottom-xl">
There are no data in this secret.
</p>
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 grammatically correct, but reads a little funny to me. Maybe use an empty state message?

<div ng-if="!secret.data" class="empty-state-message text-center">
  <h2>No data.</h2>
  <p>This secret has no data.</p>
</div>

@jhadvig
Copy link
Member Author

jhadvig commented Nov 3, 2017

@spadgett so you are right regarding the secret without data field in theEnvironment From... there is nothing to select in the value input.

bitmap

Do we want to disable the value input in that case, or just set the placeholder to just say something like "No key" ?

@spadgett
Copy link
Member

spadgett commented Nov 3, 2017

Do we want to disable the value input in that case, or just set the placeholder to just say something like "No key" ?

As long as it's not broken, we might just leave it for 3.7 and come back to it in 3.8.

cc @jwforres

@jwforres
Copy link
Member

jwforres commented Nov 3, 2017

yeah if its not throwing a JS error i wouldnt touch that code in 3.7

@spadgett
Copy link
Member

spadgett commented Nov 3, 2017

/kind bug
/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. kind/bug Categorizes issue or PR as related to a bug. labels Nov 3, 2017
@spadgett
Copy link
Member

spadgett commented Nov 3, 2017

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2017
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2017
@jhadvig
Copy link
Member Author

jhadvig commented Nov 3, 2017

@spadgett disabling the btn when no data
no-add

@spadgett
Copy link
Member

spadgett commented Nov 3, 2017

/lgtm
/hold cancel

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 3, 2017
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 1b45672 into openshift:master Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. 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.

None yet

5 participants