-
Notifications
You must be signed in to change notification settings - Fork 65
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
Update ubi9 and add ubi9-based developer image #190
Conversation
Signed-off-by: David Kwon <[email protected]>
…ckerfile Signed-off-by: David Kwon <[email protected]>
## gh-cli | ||
RUN \ | ||
TEMP_DIR="$(mktemp -d)"; \ | ||
cd "${TEMP_DIR}"; \ | ||
GH_VERSION="2.23.0"; \ | ||
GH_VERSION="2.45.0"; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was done to match the GH version in the ubi8 image:
developer-images/base/ubi8/Dockerfile
Line 42 in 0eb879a
GH_VERSION="2.45.0"; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkwon17 great work! All cases from the description work as expected. A few moments:
- In the description (step 2 and 3) should be
cd base/ubi9
andcd universal/ubi9
- Maybe we should add an information about ubi9 and udi9 into README.md and CONTRIBUTING.md files as well?
- Should we have a GH action for udi9 like
Empty workspace smoke test on udi9
?
Signed-off-by: David Kwon <[email protected]>
Signed-off-by: David Kwon <[email protected]>
Signed-off-by: David Kwon <[email protected]>
@svor thank you for the review!
Fixed
I've added a commit to change some mentions of ubi8 to ubi9. Since we are planning to deprecate ubi8: eclipse-che/che#23034 I plan to make another PR to be merged after this current PR, to remove the ubi8 code and update the rest of the readme
I've added a new workflow for this in my latest commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments, but in general LGTM!
.github/workflows/empty-worksapce-smoke-test-on-minikube-ubi9.yaml
Outdated
Show resolved
Hide resolved
.github/workflows/empty-worksapce-smoke-test-on-minikube-ubi9.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: David Kwon <[email protected]>
Signed-off-by: David Kwon <[email protected]>
Thank you @svor , I updated the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some very minor comments but overall this looks great to me, awesome work @dkwon17 :)
# Cloud | ||
|
||
# oc client | ||
ENV OC_VERSION=4.15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should update OC to the latest 4.17 (or 4.16 which is the first version of oc built specifically for RHEL 9 containers?). Though perhaps we're trying to keep as close of a 1-1 compatibility between the UBI8 and UBI9 based UDI images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it as is for this PR, and potentially upgrade in a different PR
# Configure container engine | ||
COPY --chown=0:0 containers.conf /etc/containers/containers.conf | ||
|
||
ENV K8S_VERSION=1.28 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, if we update OC we should also update to the corresponding k8s (kubectl) version that's bundled with OC.
Signed-off-by: David Kwon <[email protected]>
Signed-off-by: David Kwon <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, amazing work @dkwon17 :)
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AObuchow, dkwon17, svor The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Additional steps that should follow this PR (or could be included in it):
|
Part of eclipse-che/che#23034
To test:
First, replace line 4 of the udi dockerfile here so that the base image matches the base image built from step 2.
Then, run the following:
I've built and pushed my own images for testing here:
Case 1
The workspace starts with the base image:
Case 2
The workspace starts with the UDI image
<CHE_HOST>/#https://github.com/dkwon17/quarkus-api-example/tree/per-workspace?image=<UDI>
Next, verify that the following command runs without any errors:
Run the
Package
andBuild image
devfile tasks to verify that podman image builds are also working.Lastly, test kubedock by adding the
KUBEDOCK_ENABLED=true
env variable in the tooling container:and by running the following when the workspace restarts:
Additionally check that extensions from .vscode/extensions are installed:
Case 3
The workspace starts with the UDI image and podman build works.
After testing case 2, run the
devfile: Package
and thedevfile: Build Image
devfile tasks to build the container image:Also, verify that the
devfile: Start Development Mode
command also runs without issues: