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

remove oapi #21782

Merged
merged 4 commits into from
Feb 1, 2019
Merged

remove oapi #21782

merged 4 commits into from
Feb 1, 2019

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jan 14, 2019

realistically, I'm not going to get to this for a while.

@soltysh @sttts post-rebase I suppose,

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 14, 2019
@soltysh
Copy link
Contributor

soltysh commented Jan 14, 2019

@soltysh @sttts post-rebase I suppose,

sgtm, I can pick that up by then

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 29, 2019
@deads2k deads2k changed the title [WIP] remove oapi remove oapi Jan 29, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 29, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Jan 29, 2019

rebased and ready for review. I expect we'll bump through a few more tests

@mfojtik
Copy link
Contributor

mfojtik commented Jan 29, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2019
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 29, 2019

bumped into containers/image#570 . I'll update our fork to unstick this pull to start.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 29, 2019

@bparees is the e2e-aws-builds job currently in a good state? I may have missed a couple, but I thought I got everything under /test

@deads2k
Copy link
Contributor Author

deads2k commented Jan 29, 2019

@bparees the e2e-aws-serial job openshift-tests [registry][Serial][Suite:openshift/registry/serial] Image signature workflow can push a signed image to openshift registry and verify it [Suite:openshift/conformance/serial] Where does it get its copy of containers/image? I've got a pull to update that repo (containers/image#570), but that test is going to break without getting a bump.

@bparees
Copy link
Contributor

bparees commented Jan 29, 2019

@bparees is the e2e-aws-builds job currently in a good state? I may have missed a couple, but I thought I got everything under /test

as far as i know, other than occasional flakes until the longrunning behavior for instantiate binary is restored..I think you guys have a PR for that but i'm not sure it's merged?

@bparees
Copy link
Contributor

bparees commented Jan 30, 2019

@deads2k you can ask @gabemontero about the pipeline test failures. i know he was fixing a flake but it's also possible the pipelines use oapi somewhere, up to and including the jenkins plugins themselves.

@bparees
Copy link
Contributor

bparees commented Jan 30, 2019

@bparees the e2e-aws-serial job openshift-tests [registry][Serial][Suite:openshift/registry/serial] Image signature workflow can push a signed image to openshift registry and verify it [Suite:openshift/conformance/serial] Where does it get its copy of containers/image? I've got a pull to update that repo (containers/image#570), but that test is going to break without getting a bump.

looks like the test itself runs an image w/ skopeo in it so probably we need an updated version of skopeo that has whatever fix you've done to go against the right signatures api?

@bparees
Copy link
Contributor

bparees commented Jan 30, 2019

@gabemontero
Copy link
Contributor

gabemontero commented Jan 30, 2019

@bparees @deads2k the https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/21782/pull-ci-openshift-origin-master-e2e-aws-builds/454 failures were unrelated to the flake fix I hope to have merged Tuesday AM

These failures were from the token (oc whoami -t result)for the non-admin user set up with the extended tests failing self-sar against the namespace of the jenkins pod

The login plugin is using these uri's:

    private static final String USER_URI = "/oapi/v1/users/~";
    private static final String SAR_URI = "/oapi/v1/subjectaccessreviews";

So yeah, seems like an oapi removal would have some impact.

And I see 404's trying to hit those uri's in the jenkins log.

So confirmed.

@bparees
Copy link
Contributor

bparees commented Jan 30, 2019

@gabemontero thanks, we'll have to queue up fixing that before this can merge. Do any of the other jenkins plugins use oapi?

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, mfojtik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mtrmac
Copy link
Contributor

mtrmac commented Jan 30, 2019

@bparees the e2e-aws-serial job openshift-tests [registry][Serial][Suite:openshift/registry/serial] Image signature workflow can push a signed image to openshift registry and verify it [Suite:openshift/conformance/serial] Where does it get its copy of containers/image? I've got a pull to update that repo (containers/image#570), but that test is going to break without getting a bump.

FWIW that test could be updated at

"atomic:" + signedImage,
to use "docker://" instead of "atomic:" and it should continue to work (as long as the registry is ≥1.5.0-alpha.3); that would also allow removing the oc login call and the KUBERNETES_MASTER environment variable.

Of course none of this changes the fact that removing the namespace breaks all existing builds of skopeo when used with the atomic: transport.

@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Jan 30, 2019

/retest

1 similar comment
@deads2k
Copy link
Contributor Author

deads2k commented Jan 30, 2019

/retest

@gabemontero
Copy link
Contributor

@gabemontero
Copy link
Contributor

/test e2e-aws-builds

@soltysh
Copy link
Contributor

soltysh commented Jan 31, 2019

So I'm guessing you need #21921 to land which picks up the new new containers in vendor.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 31, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Jan 31, 2019

@bparees
Copy link
Contributor

bparees commented Feb 1, 2019

I think gabe was seeing some slowness today, yes.. i just had a successful aws-builds job run on another PR, so let's retry here.

/test e2e-aws-builds

@soltysh
Copy link
Contributor

soltysh commented Feb 1, 2019

CI failure
/retest

@deads2k
Copy link
Contributor Author

deads2k commented Feb 1, 2019

spoke with @bparees and @gabemontero the single failing build test does not appear to be related to this change and keeping the signal for changes to builds is valuable. We will manually merge this and watch the results today.

@deads2k deads2k merged commit c593f42 into openshift:master Feb 1, 2019
@@ -22,6 +22,7 @@ var _ = g.Describe("[registry][Serial][Suite:openshift/registry/serial] Image si
)

g.It("can push a signed image to openshift registry and verify it", func() {
g.Skip("disable because containers/image: https://github.com/containers/image/pull/570")
Copy link
Contributor

Choose a reason for hiding this comment

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

@cben
Copy link

cben commented May 19, 2019

Good riddance 👏 🎉.

Out of curiously, has this been publicly announced somewhere (ahead of 4.0 changelogs)?
I'll have some work to adapt ManageIQ code using openshift apis, and would have liked to learn of this earlier. Is there some public channel for "Openshift API users" to subscribe to?
(I also work in redhat, and I see there were emails I missed in the noise 😉, but I'm asking upstream angle...)

cben added a commit to cben/kubernetes-discovery-samples that referenced this pull request May 19, 2019
(Used "Red Hat-Managed" option, which is presently hidden for most
external users - it's partially, but not yet fully, what one will get
in OpenShift Dedicated 4.1.
May differ slightly from self-installed OCP?)

env DIR=openshift-4.1.0-rc.3 URL="https://api.cben-oapi.bexf.p1.openshiftapps.com:6443" WAIT_OKS="healthz readyz" tools/scrape.sh --insecure

- Notably /oapi is completely gone now!
  (openshift/origin#21782)

- TODO: /version/openshift no longer available
  (openshift/origin#21434)
  Probably need to scrape `oc get clusterversion` operator CR?
mtrmac added a commit to mtrmac/origin that referenced this pull request Feb 10, 2021
- Use docker:// instead of atomic:, as recommended back in
  openshift#21782 (comment)
  openshift#21782 (comment)
- Then re-enable the test

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac added a commit to mtrmac/origin that referenced this pull request Feb 10, 2021
- Use docker:// instead of atomic:, as recommended back in
  openshift#21782 (comment)
  openshift#21782 (comment)
- Then re-enable the test

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac added a commit to mtrmac/origin that referenced this pull request Feb 11, 2021
- Use docker:// instead of atomic:, as recommended back in
  openshift#21782 (comment)
  openshift#21782 (comment)
- Then re-enable the test

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac added a commit to mtrmac/origin that referenced this pull request Feb 11, 2021
- Use docker:// instead of atomic:, as recommended back in
  openshift#21782 (comment)
  openshift#21782 (comment)
- Then re-enable the test

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac added a commit to mtrmac/origin that referenced this pull request Mar 4, 2021
- Use docker:// instead of atomic:, as recommended back in
  openshift#21782 (comment)
  openshift#21782 (comment)

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants