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

Ensure kve does not render on browser builds if no envs to show #1970

Conversation

benjaminapetersen
Copy link
Contributor

related to #1438

There is already a good message The build strategy had no environment variables defined., this just ensures the kve doesn't render at all (rather than renders but is empty).

@spadgett

@@ -104,6 +104,7 @@ <h1 class="contains-actions">
Environment variables can be edited on the <a ng-href="{{build | configURLForResource}}?tab=environment">build configuration</a>.
</p>
<key-value-editor
ng-if="(build | buildStrategy).env"
Copy link
Member

Choose a reason for hiding this comment

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

Should be checking size to catch empty arrays?

ng-if="(build | buildStrategy).env | size"

@benjaminapetersen benjaminapetersen force-pushed the bpeterse/hide-kve-on-build-if-no-envs branch from ea373e7 to 0c06867 Compare August 31, 2017 18:23
@@ -104,6 +104,7 @@ <h1 class="contains-actions">
Environment variables can be edited on the <a ng-href="{{build | configURLForResource}}?tab=environment">build configuration</a>.
</p>
<key-value-editor
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spadgett thx, updated

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 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.

LGTM, but needs rebase

@spadgett
Copy link
Member

Hey @benjaminapetersen I think if you rebase this, we should be set to merge

@benjaminapetersen benjaminapetersen force-pushed the bpeterse/hide-kve-on-build-if-no-envs branch from 0c06867 to 6c73175 Compare September 13, 2017 18:19
@benjaminapetersen
Copy link
Contributor Author

thanks @spadgett , rebased.

@spadgett
Copy link
Member

[merge][severity: bug]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 6c73175

@openshift-bot
Copy link

openshift-bot commented Sep 13, 2017

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/193/) (Base Commit: f446b03) (PR Branch Commit: 6c73175) (Extended Tests: bug)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants