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

refactor(docker): Define tool versions only once #8648

Merged
merged 2 commits into from
May 16, 2024

Conversation

sschuberth
Copy link
Member

Previously, tool versions have been defined both inside the .versions file and as part of default ARG values. Consolidate these to ARG values only that get used where needed.

Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.92%. Comparing base (894895d) to head (16cb891).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8648   +/-   ##
=========================================
  Coverage     67.92%   67.92%           
  Complexity     1005     1005           
=========================================
  Files           244      244           
  Lines          7772     7772           
  Branches        876      876           
=========================================
  Hits           5279     5279           
  Misses         2110     2110           
  Partials        383      383           
Flag Coverage Δ
funTest-docker 66.04% <ø> (ø)
funTest-non-docker 34.79% <ø> (ø)
test 37.84% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sschuberth sschuberth force-pushed the docker-versions-include branch 2 times, most recently from 47f96b7 to c60094d Compare May 15, 2024 14:40
This is a fixup for 14cc5f1.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth force-pushed the docker-versions-include branch from c60094d to f5625d4 Compare May 15, 2024 14:58
Previously, tool versions have been defined both inside the `.versions`
file and as part of default `ARG` values. Consolidate these to `ARG`
values only that get used where needed.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth force-pushed the docker-versions-include branch from f5625d4 to 16cb891 Compare May 15, 2024 15:54
Copy link
Member

@fviernau fviernau left a comment

Choose a reason for hiding this comment

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

LGTM.

@sschuberth
Copy link
Member Author

@oheger-bosch, @MarcelBochtler, @heliocastro can I get an approval from your side?

Copy link
Contributor

@heliocastro heliocastro left a comment

Choose a reason for hiding this comment

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

Is a good approach use versions.dockerfile with include now the new syntax is available.
I think the cost of uglify with sed the github action is ok'ish.

@sschuberth
Copy link
Member Author

I think the cost of uglify with sed the github action is ok'ish.

I agree it's not super-nice, and I once filed this issue about it, but did not find the time so far to implement it.

@sschuberth sschuberth merged commit ae16d01 into main May 16, 2024
20 checks passed
@sschuberth sschuberth deleted the docker-versions-include branch May 16, 2024 05:40
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.

3 participants