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

BuildConfig editor fix #7109

Merged
merged 1 commit into from Feb 9, 2016
Merged

Conversation

jhadvig
Copy link
Member

@jhadvig jhadvig commented Feb 8, 2016

When changing the builder/output/imageSource Type from imageStreamImage || dockerImage to imageStreamTag the imageStream value has been added to the list of the imageStream options, which should be done only when the data are loaded, that means when the buildConfig has specified different imageStream than exists, to prevent loss of data.

Also renaming imageSourcesOptions to imageSourceOptions which wasnt causing any issues, but forgot to rename when was doing final updates.

@spadgett I've tested the change and should be working now as desired. PTAL.

@@ -351,7 +354,7 @@ angular.module('openshiftConsole')
$scope.imageSourceBuildFrom.tags = {};
var projectImageStreams = imageStreams.by("metadata.name");
if (!_.isEmpty(projectImageStreams)) {
if (!Object.keys(projectImageStreams).contains($scope.imageSourceBuildFrom.imageStream) && projectName === $scope.imageSourceBuildFrom.namespace) {
if (!Object.keys(projectImageStreams).contains($scope.imageSourceBuildFrom.imageStream) && projectName === $scope.imageSourceBuildFrom.namespace && !selectFirstOption) {
Copy link
Member

Choose a reason for hiding this comment

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

These should really use Object.hasOwnProperty or _.has.

!_.has(projectImageStreams, $scope.imageSourceBuildFrom.imageStream)

Not new in this PR, but worth fixing since we're changing the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 will update right away

@spadgett
Copy link
Member

spadgett commented Feb 8, 2016

LGTM except for the one comment

@jhadvig
Copy link
Member Author

jhadvig commented Feb 8, 2016

@spadgett sorry for the confusion on the irc, there are only 3 substitutions not 13.
The code is updated with them.
PTAL

@spadgett
Copy link
Member

spadgett commented Feb 8, 2016

LGTM [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/918/) (Image: devenv-rhel7_3363)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to cb63a00

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to cb63a00

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/918/)

openshift-bot pushed a commit that referenced this pull request Feb 9, 2016
@openshift-bot openshift-bot merged commit bca797f into openshift:master Feb 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants