-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 the CI runner #699
Update the CI runner #699
Conversation
.github/workflows/ci.yml
Outdated
@@ -3,9 +3,24 @@ | |||
name: WildFly Quickstarts CI | |||
|
|||
on: | |||
push: | |||
branches: | |||
- 'main' |
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.
The workflow should be "branch abstract", to work on the NN.x branches without any modifications. This is why all dependency checkouts use ref: ${{ github.base_ref }}
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.
That only kind of works. What I mean by that, is that each branch will have it's own CI. Once you branch, e.g. 29.x, then that branch runs off it's own workflow, not the one from main.
That said, this change is not needed at this point really. It's really to get around dependabot jobs from running two different CI runs. That said, there are other ways to handle that.
jobs: | ||
wildfly-build: |
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.
The workflow should be "branch abstract", to work on the NN.x branches without any modifications. This is why all dependency checkouts use ref: ${{ github.base_ref }}
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.
To be clear, the "with deps" job of the 29.x branch of QS should build with WFLY 29.x and BOMs 29.x built with such WFLY.
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.
I guess in this case we could simply use the following?
uses: wildfly/wildfly/.github/workflows/shared-wildfly-build.yml
with:
wildfly-branch: ${{ github.base_ref }}
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.
Yes, we could change the ${{ github_base_ref}}
here for sure. I'll fix that.
with: | ||
path: quickstarts | ||
- name: Set up JDK ${{ matrix.java }} | ||
uses: AdoptOpenJDK/install-jdk@v1 | ||
- uses: actions/download-artifact@v3 |
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.
What's the purpose of this artifact download?
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.
Ok nevermind I saw in the WFLY CI workflow sources this is related with the reusable workflow. I should note that this makes the job look more complex / less readable than the simple checkout and mvn build tho ;-)
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.
By the way, is it possible to start the WFLY built and downloaded this way?
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.
There is no WildFly distribution built here, it's the maven repository which contains the dependencies required for provisioning a SNAPSHOT of WildFly for testing.
I've pushed changes to not use |
b475d54
to
d19afb7
Compare
There is not a dynamic way to use the branch for the reusing the wildfly-build. I've hard-coded it to main for now, but it would need a change once we branch. However, I do feel personally that is okay. If it's not something you want to do though then we can just ditch this PR. IMO it's good to share builds in case things change there is only one place they need to change. |
…e newer option. Signed-off-by: James R. Perkins <[email protected]>
Signed-off-by: James R. Perkins <[email protected]>
… any special settings requiring the AdoptOpenJDK action. Signed-off-by: James R. Perkins <[email protected]>
Signed-off-by: James R. Perkins <[email protected]>
Signed-off-by: James R. Perkins <[email protected]>
@jamezp I will need to discuss this later with you, since this conflicts with what's coming wrt CI from https://issues.redhat.com/browse/WFLY-18411 (PoC currently at #696 but I will extract a PR taking out microprofile-config). We probably could do a 5-10 min meet about it.... |
#696 was merged now and ci.yml is now project_ci.yml, and most of the enhancements here are there already. What's missing is the usage of WFLY workflow to build it, which I would like to further discuss it with you. Once you get back from PTO please ping me wrt that. |
@jamezp: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
Closing since those enhancements are now all being used by the new CI workflows |
Also add dependabot for upgrading GitHub Actions.