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

JBOSS EAP 8.1 images in Konflux POC #44

Open
wants to merge 1 commit into
base: eap81-beta-dev
Choose a base branch
from

Conversation

rdnovell
Copy link
Contributor

@rdnovell rdnovell commented Jan 14, 2025

This will add a Github cekit action. Cekit will run in dry-run mode and the genereated files will be copied and commited.

Issue: https://issues.redhat.com/browse/CLOUD-4265
Signed-off-by: Ruben Novelli [email protected]

@rdnovell rdnovell requested review from jmesnil and jfdenise January 14, 2025 13:04
Copy link
Member

@jmesnil jmesnil left a comment

Choose a reason for hiding this comment

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

@rdnovell Thanks, I've added some comments to better understand what is produced and how we would be able to build images from the generated container files.


- name: Create folders
run: |
mkdir -p container-images/jdk17/runtime/
Copy link
Member

Choose a reason for hiding this comment

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

We could use a matrix to run the same steps for either jdk17 or jdk21, that'd simplify this file.

@@ -0,0 +1,51 @@
name: Publish cekit
on:
workflow_dispatch
Copy link
Member

Choose a reason for hiding this comment

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

for POC, that's fine to run this action manually.
When do you plan to run it? On opened PRs, on merges?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From a talk with @jfdenise we will do:
open the PR: , dry-run 4 times, run the tests (no tests run currently).
merge the PR: dry-run 4 times, commit/push the changes.

My idea is use this pattern

on:
  pull_request:
    branches: 
      - eap81-beta-dev

jobs:
  main-job:
    ...
  commit-job:
    if: github.event.pull_request.merged == true
    ...

run: cekit --descriptor ./builder-image/image.yaml build --overrides ./builder-image/image-jdk21-overrides.yaml --dry-run docker
- name: Copy files to container-images folder
run: |
cp -prf target/image/Dockerfile container-images/jdk21/builder
Copy link
Member

Choose a reason for hiding this comment

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

At this point, we have an up-to-date Dockerfile in container-images/jdk21/builder but this file is referencing cekit modules (eg COPY modules/jboss.container.user .... What'd be the build context to build this images?
Does cekit dry-run also fetch cekit modules? Should they be added to the git commit below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cekit creates a modules folder, I need to copy this folder too. @jfdenise, @jmesnil do you prefer to have a shared module folder or one module folder in each container-images/jdkXX/[runtime|builder] ?


- name: Push the new files
run: |
git config --global user.email "[email protected]"
Copy link
Member

Choose a reason for hiding this comment

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

We'll have to decide how these automated commits are identified.

run: |
git config --global user.email "[email protected]"
git config --global user.name "Ruben Novelli"
git add container-images
Copy link
Member

Choose a reason for hiding this comment

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

should we do a git add .?

git config --global user.name "Ruben Novelli"
git add container-images
git commit -m "Sync files"
git push
Copy link
Member

Choose a reason for hiding this comment

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

if the action is run when a PR is opened, would that push to the HEAD of the PR branch? That might be problematic if we want to rebase/amend PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to add if: github.event.pull_request.merged == true and also the correct branch

###### START module 'jboss.container.user:2.0+jboss1'
###### \
# Copy 'jboss.container.user' module content
COPY modules/jboss.container.user /tmp/scripts/jboss.container.user
Copy link
Member

Choose a reason for hiding this comment

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

Here, what is the build context so that we can COPY the modules/jboss.container.user directory?

@jfdenise
Copy link
Contributor

I thin ktha tthis PR should be put in Draft for now. @rdnovell is investigating and it seems not ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants