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

CI updates for PKI 11.5 #4678

Merged
merged 4 commits into from
Feb 29, 2024
Merged

CI updates for PKI 11.5 #4678

merged 4 commits into from
Feb 29, 2024

Conversation

edewata
Copy link
Contributor

@edewata edewata commented Feb 27, 2024

  • The version numbers for PKI 11.5 container images have been changed to 11.5.
  • The default BASE_IMAGE for PKI 11.5 has been changed to Fedora 40.
  • The default COPR_REPO for PKI 11.5 has been removed since all dependencies should be available on Fedora 40.

The version numbers for PKI 11.5 container images have been
changed to 11.5.
The default BASE_IMAGE for PKI 11.5 has been changed to Fedora 40.
The default COPR_REPO for PKI 11.5 has been removed since all
dependencies should be available on Fedora 40.
@edewata
Copy link
Contributor Author

edewata commented Feb 27, 2024

Some of the CI failures are known issues on Fedora 40: #4667

This one is happening on Fedora 39 too, but it doesn't seem to be caused by this PR:
https://github.com/dogtagpki/pki/actions/runs/8057343631/job/22011672520#step:23:267

Copy link
Member

@fmarco76 fmarco76 left a comment

Choose a reason for hiding this comment

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

This PR is not clear to me. With this change we have to update at every Fedora/pki release these values. I think it was better to default at latest version (or rawhide) and set the BASE_IMAGE variable to the needed version so we have to change only in one place. Is there a specific reason to not set the variable?

The publishing jobs for PKI 11.5 have been updated to run on
v11.5 branch only.
Copy link

sonarcloud bot commented Feb 27, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@edewata
Copy link
Contributor Author

edewata commented Feb 27, 2024

(I just added another commit to fix the publishing jobs.)

This is something that we usually do after branching to make sure the CI will still work in the future regardless of changes in Fedora, but I'm open to ideas if we can improve it. Here's what we've done for v11.4 branch:

The publishing job need to run whenever there's a change in v11.4 branch instead of master branch:
https://github.com/dogtagpki/pki/blob/v11.4/.github/workflows/publish.yml#L6

The containers need to be published with a 11.4 version number so they don't override containers built from the master branch:
https://github.com/dogtagpki/pki/blob/v11.4/.github/workflows/publish.yml#L98-L116

The default BASE_IMAGE was changed to Fedora 39 since PKI 11.4 was released on Fedora 39 and it might have issues on Fedora 40 or later (but it can still be changed using GH variable):
https://github.com/dogtagpki/pki/blob/v11.4/Dockerfile#L14

The default COPR_REPO was removed since we don't actually use it anymore, and if it keeps pointing to @pki/master repo it will have a problem in the future since at some point the repo will no longer support Fedora 39 (but it can still be changed using GH variable):
https://github.com/dogtagpki/pki/blob/v11.4/Dockerfile#L15

@fmarco76
Copy link
Member

This is something that we usually do after branching to make sure the CI will still work in the future regardless of changes in Fedora, but I'm open to ideas if we can improve it. Here's what we've done for v11.4 branch:

This sound good but in this case this PR should be against v11.5 branch and not master. Is this right?

@edewata edewata changed the base branch from master to v11.5 February 28, 2024 15:50
@edewata
Copy link
Contributor Author

edewata commented Feb 28, 2024

Ah yes! Sorry about that. It's fixed now.

Copy link
Member

@fmarco76 fmarco76 left a comment

Choose a reason for hiding this comment

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

LGTM

@fmarco76
Copy link
Member

Ah yes! Sorry about that. It's fixed now.

Thanks for the change. Now it is more clear to me how old branch CI works, I have not investigated in the past. However, I am wondering if we could use environment variable like BASE_IMAGE_<branch_name> so we need to create the variable before branching and no change in the actions. Maybe in future action updates

@edewata
Copy link
Contributor Author

edewata commented Feb 29, 2024

@fmarco76 Thanks! I'm not that clear how BASE_IMAGE_<branch_name> would work, but we can certainly discuss it when we're ready to make CI improvements.

@edewata edewata merged commit 27e7dfc into dogtagpki:v11.5 Feb 29, 2024
130 of 135 checks passed
@fmarco76
Copy link
Member

fmarco76 commented Mar 1, 2024

@edewata I was reading some comments on how to get different variables depending on the branch name but not sure if and how it works. I'll do some tests when possible.

@fmarco76
Copy link
Member

fmarco76 commented Mar 1, 2024

As an update we could also look at environments: https://github.com/orgs/community/discussions/56123

@edewata
Copy link
Contributor Author

edewata commented Mar 1, 2024

I've never used GH environments, but I don't think we should require a separate environment for every branch because they will accumulate, and also that will make it more difficult for someone to clone the repo to run the CI since they will need to set up similar environments too in their repo. I think GH environments (or environment variables) should be optional and the CI should work without them. We probably need to use something like this:

env:
  BASE_IMAGE: ${{ vars.BASE_IMAGE || github.ref_name == 'master' && 'fedora:latest' || 'fedora:40' }}

It's a bit too long for my liking, but at least it should work in the master branch and we don't need to change it when we create the v<x>.<y> branch.

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.

None yet

2 participants