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

Docker image with git sha only for pushes on main branch #155

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented Sep 3, 2024

What

Temporary fix as docker images with git sha tags from branches other than main is causing issues on the nightly build of kuadrant. TL;RD: kuadrant nightly release pipeline is picking the latest (based on last_modified attribute) image with git sha like tags. When limitador operator docker images with git sha tag are being created for branches, those were sometimes being picked for nightly tests instead of the HEAD of main.

@eguzki eguzki requested a review from didierofrivia September 3, 2024 17:57
@eguzki eguzki changed the title Fix image build Docker image with git sha only for pushes on main branch Sep 3, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.01%. Comparing base (248c5c9) to head (cf8e637).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
- Coverage   85.91%   85.01%   -0.91%     
==========================================
  Files          19       19              
  Lines         994      994              
==========================================
- Hits          854      845       -9     
- Misses         92       97       +5     
- Partials       48       52       +4     
Flag Coverage Δ
integration 79.37% <ø> (-1.11%) ⬇️
unit 66.51% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1alpha1 (u) 100.00% <ø> (ø)
pkg/helpers (u) 83.87% <ø> (ø)
pkg/log (u) 94.73% <ø> (ø)
pkg/reconcilers (u) 74.67% <ø> (ø)
pkg/limitador (u) 98.11% <ø> (ø)
controllers (i) 75.33% <ø> (-3.00%) ⬇️
pkg/upgrades 88.88% <ø> (ø)

see 1 file with indirect coverage changes

Copy link
Member

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

🍰

if: ${{ github.ref_name == env.MAIN_BRANCH_NAME }}
id: add-git-sha-tag
run: |
echo "IMG_TAGS=${{ github.sha }} ${{ env.IMG_TAGS }}" >> $GITHUB_ENV
Copy link
Contributor

@guicassolato guicassolato Sep 4, 2024

Choose a reason for hiding this comment

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

I understand Build images for main branch will trigger a build when commits are pushed to main. By not specifying operatorTag, these builds will be tagged latest (default) and <commit-sha> (added in this line).

Build images for dev branches will NOT trigger a build for main due to the if condition added (to make sure only pushes to feature branches will do it). However, Schedule build with latest image SHA versions will trigger another build, and by specifying operatorTag = <commit-sha>, these images will have the <commit-sha> tag only (redundantly added here) and will separate from latest (previously built, on a different manifest).

Q: Do we need to keep Schedule build with latest image SHA versions? Is it because it specifies limitadorVersion? If we are to keep it, can we please remove the operatorTag param there (so it assumes latest by default)? In this case, would we still need Build images for main branch?

Copy link
Contributor

@guicassolato guicassolato Sep 4, 2024

Choose a reason for hiding this comment

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

IMO, we probably want to keep Build images for main branch and get rid of Schedule build with latest image SHA versions. Maybe add limitadorVersion to the former?

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 was about to delete the scheduled build with the latest image with the git sha tag. But then I realized that this build is actually the correct one. The build triggered from push to main is not correct.

We need to fix this in a following up PR

That's good enough, @guicassolato ?

Copy link
Contributor

Choose a reason for hiding this comment

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

IDK. I don't really like that main is built twice today. I would get rid of the scheduled one right away and add limitadorVersion: ${{ vars.LIMITADOR_SHA }} to the build main one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! Too soon. I see the problem with that now... When the operand changes, nothing will rebuild the operator with the updated SHA, right? Damn!

Copy link
Contributor

Choose a reason for hiding this comment

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

So add limitadorVersion: ${{ vars.LIMITADOR_SHA }} to build main and remove this line but keep both workflows? main will build twice but at least both tags will be added and linked every time. Right?

Copy link
Contributor Author

@eguzki eguzki Sep 4, 2024

Choose a reason for hiding this comment

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

Oh! Too soon. I see the problem with that now... When the operand changes, nothing will rebuild the operator with the updated SHA, right? Damn!

Right.

The other option is to trigger build on the operator project when the operand gets a new build. But I rather prefer the nightly build approach instead. To avoid too many operator builds in the same day when the operand changes many times in the same day.

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 would like to fix that in another PR

@eguzki eguzki merged commit 8d7e9ed into main Sep 4, 2024
16 checks passed
@eguzki eguzki deleted the fix-image-build branch September 4, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants