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

Publish image for infinispan-test, replaces Jenkins stage 'Image' #232

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

rigazilla
Copy link
Contributor

Workflow to build and publish infinispan server image on quay.io/infinispan-test

Copy link
Collaborator

@ryanemerson ryanemerson left a comment

Choose a reason for hiding this comment

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

Thanks @rigazilla, overall LGTM just a few minor things. Do you have a link to the corresponding Infinispan action? If I can see the user of this, it will give me the full context to review more thoroughly.

.github/workflows/on_call_do_publish_image.yml Outdated Show resolved Hide resolved
.github/workflows/on_call_do_publish_image.yml Outdated Show resolved Hide resolved
.github/workflows/on_call_do_publish_image.yml Outdated Show resolved Hide resolved
.github/workflows/on_call_do_publish_image.yml Outdated Show resolved Hide resolved
.github/workflows/on_call_do_publish_image.yml Outdated Show resolved Hide resolved
@ryanemerson
Copy link
Collaborator

Very minor nitpick, I would rename the action to just publish_image.yml as we have both call and dispatch targets.

@rigazilla
Copy link
Contributor Author

Thanks @rigazilla, overall LGTM just a few minor things. Do you have a link to the corresponding Infinispan action? If I can see the user of this, it will give me the full context to review more thoroughly.

It will be based on this: https://github.com/rigazilla/infinispan/blob/main/.github/workflows/on_run_publish_image.yml
it needs some updates first

@rigazilla
Copy link
Contributor Author

Very minor nitpick, I would rename the action to just publish_image.yml as we have both call and dispatch targets.

yeah ok, the naming convention I'm trying to apply doesn't work very well... too verbose

@ryanemerson
Copy link
Collaborator

It will be based on this: https://github.com/rigazilla/infinispan/blob/main/.github/workflows/on_run_publish_image.yml
it needs some updates first

Thanks, that's really helpful.

So looking at you current code, it seems like for PRs we need to wait for all tests to run before the image is built. This makes sense for pushes to branches, but would it be possible to make it so that the workflow is triggered on PR open? Ideally I wouldn't have to wait ~1h before the image build happens.

@rigazilla
Copy link
Contributor Author

It will be based on this: https://github.com/rigazilla/infinispan/blob/main/.github/workflows/on_run_publish_image.yml
it needs some updates first

Thanks, that's really helpful.

So looking at you current code, it seems like for PRs we need to wait for all tests to run before the image is built. This makes sense for pushes to branches, but would it be possible to make it so that the workflow is triggered on PR open? Ideally I wouldn't have to wait ~1h before the image build happens.

oh that was my intent, but is gone lost when I moved out the publishing code from the main workflow. I'll work on that

if this PR looks good, is it ok for a merge?
this way I can point the main branch instead of this in the other workflows and simplify things

run-id:
description: 'Id of the run to download server artifact from'
type: string
serverArtifact:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mind whether we use camelCase or kebab-case, but let's try to be consistent with the input element across our various workflows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's go with camel

@ryanemerson
Copy link
Collaborator

if this PR looks good, is it ok for a merge?

Do you have a link to a successful run from your fork?

@rigazilla
Copy link
Contributor Author

if this PR looks good, is it ok for a merge?

Do you have a link to a successful run from your fork?

yep, I'm running on my fork for now:

published images

@ryanemerson ryanemerson merged commit efeb437 into infinispan:main Sep 16, 2024
3 checks passed
@ryanemerson
Copy link
Collaborator

Thanks @rigazilla

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.

2 participants