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

Include namespace in determining new-app dup objects #4297

Merged
merged 1 commit into from
Aug 21, 2015

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented Aug 20, 2015

Fixes #3947

@csrwng
Copy link
Contributor Author

csrwng commented Aug 20, 2015

@jhadvig @bparees ptal

@smarterclayton
Copy link
Contributor

What if I'm creating an app from an image stream in the current namespace where the output name is the same as the input name? oc new-build --image-stream=foo --name foo?

@csrwng
Copy link
Contributor Author

csrwng commented Aug 20, 2015

you'll create a build loop

@csrwng
Copy link
Contributor Author

csrwng commented Aug 20, 2015

should we stop you from shooting yourself in the foot?

@smarterclayton
Copy link
Contributor

In this case, i don't know what the user expects, but they probably expect something. I only bring it up because there is an existing item and a dup in the same namespace, but the new object probably doesn't have namespace set while the old one does. So the acceptor won't think they're duplicates.

@csrwng
Copy link
Contributor Author

csrwng commented Aug 20, 2015

ic, hmm, we wouldn't catch it... so you'd get an error saying it couldn't be saved because it's a dup, which I guess is ok?

@smarterclayton
Copy link
Contributor

maybe?

@bparees
Copy link
Contributor

bparees commented Aug 20, 2015

why doesn't the new object have namespace set?

lgtm but not merging pending the outcome of this discussion.

@smarterclayton
Copy link
Contributor

Mostly so you can -o yaml it and use it on another namespace.

On Aug 20, 2015, at 6:33 PM, Ben Parees notifications@github.com wrote:

why doesn't the new object have namespace set?

lgtm but not merging pending the outcome of this discussion.


Reply to this email directly or view it on GitHub
#4297 (comment).

@bparees
Copy link
Contributor

bparees commented Aug 21, 2015

hm. would it make more sense to strip the namespace out when outputting yaml, instead of not setting one?

@csrwng
Copy link
Contributor Author

csrwng commented Aug 21, 2015

Hmm, not sure that I see much benefit to it. IMHO that you get an error
saying you couldn't create the image stream because there's one with the
same name is ok.

On Thu, Aug 20, 2015 at 9:35 PM, Ben Parees notifications@github.com
wrote:

hm. would it make more sense to strip the namespace out when outputting
yaml, instead of not setting one?


Reply to this email directly or view it on GitHub
#4297 (comment).

@bparees
Copy link
Contributor

bparees commented Aug 21, 2015

works for me. [merge]

@csrwng
Copy link
Contributor Author

csrwng commented Aug 21, 2015

I think we need to get new-app handle existing resources more intelligently like warning you about duplicates and giving you options to overwrite/ignore/merge, but we should create a card for it :-)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 40566ce

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 40566ce

@smarterclayton
Copy link
Contributor

Yeah, that's before we save but after we would output. But technically
that's just reconciliation, so hopefully upstream will sort that out soon.

On Thu, Aug 20, 2015 at 9:45 PM, Cesar Wong notifications@github.com
wrote:

I think we need to get new-app handle existing resources more
intelligently like warning you about duplicates and giving you options to
overwrite/ignore/merge, but we should create a card for it :-)


Reply to this email directly or view it on GitHub
#4297 (comment).

Clayton Coleman | Lead Engineer, OpenShift

@openshift-bot
Copy link
Contributor

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

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3061/) (Image: devenv-fedora_2191)

openshift-bot pushed a commit that referenced this pull request Aug 21, 2015
@openshift-bot openshift-bot merged commit ab0b621 into openshift:master Aug 21, 2015
@csrwng csrwng deleted the newapp_dedup_imgstream branch August 26, 2015 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants