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: Use correct helper image even if both are built simultaneously #2606

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

mqasimsarfraz
Copy link
Member

@mqasimsarfraz mqasimsarfraz commented Mar 11, 2024

Currently, if both helper images are built together one of the images will have a default value since we set tag for both the images in each matric run. This change ensures we only set the default value when needed and not in each matric run.

I mainly hit it when doing some testing on the fork as mentioned below. Also, it was always the dnstester which is affected since it is done building before ebpf-builder. Perhaps something that will happen rarely in main repo but still a weird issue.

Fixes: 5ddb3fb

Testing done

w/o the change

with the change

  • Run with both images built together can be found here. You can see it is using custom image for both (ebpf-builder, dnstester the cases correctly.

Copy link
Member

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

Hi!

I understand the problem and you are indeed tackling it, but we do not need two steps for this, see my comment.
Also can you please check the commit message.

Best regards.

.github/workflows/inspektor-gadget.yml Outdated Show resolved Hide resolved
Fixes: 5ddb3fb

Currently, if both images are built together one of the
image will have a default value since we set tag for
both the images in each matric run. This change ensures
we only set the default value when needed and not in each
matric run.

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
Copy link
Member

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

Looks good from CI point of view! Thank you for the fix!

@mqasimsarfraz
Copy link
Member Author

Thanks for the review!

@mqasimsarfraz mqasimsarfraz merged commit 221aea3 into main Mar 12, 2024
59 checks passed
@mqasimsarfraz mqasimsarfraz deleted the qasim/fix-helper-images branch March 12, 2024 13:14
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