Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Nov 20, 2019

Lean on the path consistency from openshift/oc@e949088d (openshift/oc#153) to make it easier to scale as we add/remove arch/OS support. With this commit, the downloads deployment is less opinionated about what is in the cli-artifacts bundle. It looks for all possible arch/OS combinations, serves what it finds, and 404s requests for binaries it could not find.

I've also renamed mac -> darwin in the canonical URIs, so there's no mental overhead for translating from the conventional GOOS/GOARCH. I've added symlinks in the pod to support external folks who had hard-coded the old URIs, but we should be able to drop those symlinks after a reasonable deprecation period. I have not overhauled the server to 301 requests to the deprecated URI, and we probably want that before dropping the symlinks.

CC @yselkowitz, previous discussion here and in later comments in that thread.

Lean on the path consistency from openshift/oc@e949088d (Enable all
Linux arches in cli-artifacts, 2019-11-07, openshift/oc#153) to make
it easier to scale as we add/remove arch/OS support.  With this
commit, the downloads deployment is less opinionated about what is in
the cli-artifacts bundle.  It looks for all possible arch/OS
combinations, serves what it finds, and 404s requests for binaries it
could not find.

I've also renamed mac -> darwin in the canonical URIs, so there's no
mental overhead for translating from the conventional GOOS/GOARCH [1].
I've added symlinks in the pod to support external folks who had
hard-coded the old URIs, but we should be able to drop those symlinks
after a reasonable deprecation period.  I have not overhauled the
server to 301 requests to the deprecated URI, and we probably want
that before dropping the symlinks.

[1]: https://golang.org/doc/install/source#environment
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 20, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wking
To complete the pull request process, please assign spadgett
You can assign the PR to them by writing /assign @spadgett in a comment when ready.

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

Details 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

@openshift-ci-robot
Copy link
Contributor

@wking: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-gcp 0030b57 link /test e2e-gcp
ci/prow/e2e-aws-operator 0030b57 link /test e2e-aws-operator

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wking added a commit to wking/oc that referenced this pull request Nov 21, 2019
These are where the binaries lived before e949088 (Enable all Linux
arches in cli-artifacts, 2019-11-07, openshift#153), and since the extraction
chokes on hardlinks [1], we need to be looking at those locations to
extract from 4.2.z and older releases.

Also shuffles the Dockerfile around to move the darwin and Windows
builds to their old locations and symlink them from the standard
locations.  That way older 4.2.z releases will find regular files at
the locations they expect, but that we'll be able to serve files from
the standardized pattern in the downloads deployment [2].

I'm also removing the cli-artifacts-built linux/amd64 binary in favor
of the build we inherited from the parent cli image.

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1774642
[2]: openshift/console-operator#354
@benjaminapetersen
Copy link
Contributor

Needs a rebase.

@openshift-ci-robot
Copy link
Contributor

@wking: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2019
@jhadvig
Copy link
Member

jhadvig commented Dec 3, 2019

@wking could you please rebase this PR

@wking
Copy link
Member Author

wking commented Dec 5, 2019

could you please rebase this PR

Sorry, missed these. And since openshift/oc#171 partially walked back the standardized paths, this approach will no longer work without some touchups. I'll close until we get openshift/oc#172 or similar to restore the standardized paths which would let us revive this approach.

@wking wking closed this Dec 5, 2019
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants