Skip to content

oc should not fail during version negotiation against kube server#10824

Merged
openshift-bot merged 2 commits intoopenshift:masterfrom
smarterclayton:fix_version
Sep 7, 2016
Merged

oc should not fail during version negotiation against kube server#10824
openshift-bot merged 2 commits intoopenshift:masterfrom
smarterclayton:fix_version

Conversation

@smarterclayton
Copy link
Copy Markdown
Contributor

@smarterclayton smarterclayton commented Sep 6, 2016

If /oapi is not found, properly exit. Also don't shortcircuit in version

@liggitt review please for 1.3

@smarterclayton
Make Kube version info best effort, and don't fail fast
07c71da 
Try to fetch both, starting with Kube
@smarterclayton
Don't fail during negotiation when /oapi is missing
e2a63d2 
Return the requested version from the client and let the failure happen
later. Needs to be revisited in the future once we have better control
over the negotiation process.
@smarterclayton smarterclayton added this to the 1.3.0 milestone Sep 6, 2016
@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Sep 6, 2016

[test]

@smarterclayton smarterclayton mentioned this pull request Sep 6, 2016
Closed
@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Sep 6, 2016

Spawned from #10821

@liggitt
Copy link
Copy Markdown
Contributor

liggitt commented Sep 6, 2016

Will look tonight

@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Sep 6, 2016

[test] #8427

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Sep 6, 2016

Evaluated for origin test up to e2a63d2

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Sep 6, 2016

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

liggitt
liggitt reviewed Sep 7, 2016
View reviewed changes
pkg/cmd/cli/cmd/version.go
return
}

ocVersionBody, err := oClient.Get().AbsPath("/version/openshift").Do().Raw()
Copy link
Copy Markdown
Contributor

@liggitt liggitt Sep 7, 2016

Choose a reason for hiding this comment

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

won't the oClient, kClient, err := o.Clients() call above fail on a non-openshift server?

Copy link
Copy Markdown
Contributor Author

@smarterclayton smarterclayton Sep 7, 2016

Choose a reason for hiding this comment

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

not anymore. The only failure was from negotiation - david and I discussed
and it seemed reasonable that if you can't hit /oapi we might as well just
use whatever you passed - subsequent calls will really fail.

On Tue, Sep 6, 2016 at 9:10 PM, Jordan Liggitt notifications@github.com
wrote:

In pkg/cmd/cli/cmd/version.go
#10824 (comment):

@@ -128,38 +128,37 @@ func (o VersionOptions) RunVersion() error {
return
}

  •   ocVersionBody, err := oClient.Get().AbsPath("/version/openshift").Do().Raw()

won't the oClient, kClient, err := o.Clients() call above fail on a
non-openshift server?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/10824/files/e2a63d2b900e2b5da5ba40d13651444f35d49e3a#r77746479,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p6W3M9jgd2KfpCw8jcB_NSmuzmlgks5qng8bgaJpZM4J2Nxd
.

@liggitt
Copy link
Copy Markdown
Contributor

liggitt commented Sep 7, 2016

LGTM

@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Sep 7, 2016

[merge]

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Sep 7, 2016

Evaluated for origin merge up to e2a63d2

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Sep 7, 2016 •
edited
Loading

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

@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Sep 7, 2016

Cute

--- FAIL: TestImageStreamAdmitStatusUpdate (7.09s)
    imagestream_admission_test.go:561: creating limit range object "limits" with openshift.io/ImageStream limited to: map[openshift.io/image-tags:{{0 0} {<nil>} 0 DecimalSI} openshift.io/images:{{0 0} {<nil>} 0 DecimalSI}]
    imagestream_admission_test.go:376: trying to create a new image stream with a tag exceeding limit map[openshift.io/image-tags:{{0 0} {<nil>} 0 DecimalSI} openshift.io/images:{{0 0} {<nil>} 0 DecimalSI}]
    imagestream_admission_test.go:382: adding new tag to image stream status exceeding limit map[openshift.io/image-tags:{{0 0} {<nil>} 0 DecimalSI} openshift.io/images:{{0 0} {<nil>} 0 DecimalSI}]
    imagestream_admission_test.go:570: bump a limit on resource "openshift.io/images" to 1
    imagestream_admission_test.go:408: adding new tag to image stream status below limit map[openshift.io/image-tags:{{0 0} {<nil>} 0 DecimalSI} openshift.io/images:{{0 0} {<nil>} 0 DecimalSI}]
    imagestream_admission_test.go:423: unexpected error: ImageStream "is" is invalid: imageStream: Forbidden: ImageStream "is" is forbidden: requested usage of openshift.io/images exceeds the maximum limit per openshift.io/ImageStream (1 > 0)
FAIL

@openshift-bot openshift-bot merged commit 884520c into openshift:master Sep 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@smarterclayton @liggitt @openshift-bot