Skip to content

Install oc#2777

Merged
jlebon merged 2 commits intocoreos:mainfrom
cgwalters:ocp-assembler
Mar 29, 2022
Merged

Install oc#2777
jlebon merged 2 commits intocoreos:mainfrom
cgwalters:ocp-assembler

Conversation

@cgwalters
Copy link
Copy Markdown
Member

@cgwalters cgwalters commented Mar 24, 2022

The immediate motivation is https://github.com/openshift/release/blob/master/ci-operator/config/openshift/os/openshift-os-master.yaml#L77

For RHEL CoreOS related stuff in particular it's just really useful to have
the oc binary in coreos-assembler to streamline things like
"build a new release image that overrides the OS" etc.

I suspect this would help OKD too.


@cgwalters
Copy link
Copy Markdown
Member Author

(Feel free to bikeshed on naming...rhcos-assembler:latest? Doesn't quite fit since OKD could want this too. Maybe ocp-coreos-assembler:latest?)

@jlebon
Copy link
Copy Markdown
Member

jlebon commented Mar 28, 2022

Or should we just ship oc in cosa purely for practicality? Even if nothing in cosa actually uses it (though cosa oc-adm-release could).

@cgwalters
Copy link
Copy Markdown
Member Author

Or should we just ship oc in cosa purely for practicality? Even if nothing in cosa actually uses it

I'm a bit uncertain. One thought I have here is that in the future, the version of oc we use should actually be driven by OCP branches which we already have (except they're called "rhcos-"). And mechanically, I was thinking we may find ourselves wanting to add more into coreos-assembler that is actually OpenShift specific. (Although OTOH if we do a lot of it, it could become a separate git repository)

(though cosa oc-adm-release could).

Ah yeah, I should have called that out in this PR. Hmm. That code almost seems like it should live in github.com/openshift/coreos-assembler or so. If we did that, then we could have this oc injection live there too, and cleanly use all the same Prow tooling there, e.g. auto-create release-4.X git branches, etc.

A lot of tradeoffs (advantages/disadvantages).

Just adding oc into the existing image is definitely easier.

So...hmm. I dunno. I guess, does anyone object to just adding in $current oc into the image?

(Side note, looks like https://src.fedoraproject.org/rpms/origin/blob/rawhide/f/origin.spec hasn't been updated in nearly 2 years)

@cgwalters cgwalters changed the title Add Dockerfile.ocp Install oc Mar 28, 2022
@cgwalters
Copy link
Copy Markdown
Member Author

Just adding oc into the existing image is definitely easier.

Done!

jlebon
jlebon previously approved these changes Mar 28, 2022
Copy link
Copy Markdown
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM! Would be good to have at least one other person chime in.

miabbott
miabbott previously approved these changes Mar 28, 2022
Copy link
Copy Markdown
Member

@miabbott miabbott left a comment

Choose a reason for hiding this comment

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

I like this approach. LGTM!

The immediate motivation is https://github.com/openshift/release/blob/master/ci-operator/config/openshift/os/openshift-os-master.yaml#L77

For RHEL CoreOS related stuff in particular it's just really useful to have
the `oc` binary in coreos-assembler to streamline things like
"build a new release image that overrides the OS" etc.

I suspect this would help OKD too.
@cgwalters cgwalters dismissed stale reviews from miabbott and jlebon via 1f4a67b March 28, 2022 22:25
@jlebon
Copy link
Copy Markdown
Member

jlebon commented Mar 29, 2022

[2022-03-28T22:36:11.235Z] src/cmd-oc-adm-release:18:1: F401 'shutil' imported but unused
[2022-03-28T22:36:11.235Z] src/cmd-oc-adm-release:20:1: F401 'tarfile' imported but unused
[2022-03-28T22:36:11.235Z] src/cmd-oc-adm-release:21:1: F401 'tempfile' imported but unused
[2022-03-28T22:36:11.235Z] src/cmd-oc-adm-release:22:1: F401 'urllib.request' imported but unused
[2022-03-28T22:36:11.235Z] make: *** [Makefile:44: flake8] Error 1
script returned exit code 2

We will have it by default.
@cgwalters
Copy link
Copy Markdown
Member Author

[2022-03-28T22:36:11.235Z] src/cmd-oc-adm-release:18:1: F401 'shutil' imported but unused

I am so done with dynamic programming languages 😦

@jlebon jlebon enabled auto-merge (rebase) March 29, 2022 16:23
@jlebon jlebon merged commit 6f6a5a3 into coreos:main Mar 29, 2022
miabbott added a commit that referenced this pull request Apr 28, 2022
When running the `rhcos.upgrade.from-ocp-rhcos` test in an
unprivileged pod in the Prow clusters, the test fails during the
unpacking of the `oc` binary because it is trying to use `sudo` to
write to `/usr/bin`.

But since #2777 we have
the `oc` binary installed in `cosa`, so just drop the code doing the
install of the binary.
miabbott added a commit that referenced this pull request Apr 28, 2022
When running the `rhcos.upgrade.from-ocp-rhcos` test in an
unprivileged pod in the Prow clusters, the test fails during the
unpacking of the `oc` binary because it is trying to use `sudo` to
write to `/usr/bin`.

But since #2777 we have
the `oc` binary installed in `cosa`, so just drop the code doing the
install of the binary.

(cherry picked from commit 37f6c2a)
@jlebon
Copy link
Copy Markdown
Member

jlebon commented Nov 25, 2022

/cherrypick rhcos-4.9-new

@openshift-cherrypick-robot
Copy link
Copy Markdown

@jlebon: #2777 failed to apply on top of branch "rhcos-4.9-new":

Applying: Install `oc`
Using index info to reconstruct a base tree...
M	build.sh
Falling back to patching base and 3-way merge...
Auto-merging build.sh
CONFLICT (content): Merge conflict in build.sh
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Install `oc`
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherrypick rhcos-4.9-new

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.

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