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

chore: Simplify coverage dockerfile. #2679

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Feb 11, 2024

Use per-dockerfile dockerignore instead of sources for coverage. It's a bit more duplication (dockerignore is copied out of the sources one), but it's a bit more efficient and works in more situations (e.g. one where you can only build 1 dockerfile such as on render.com).


This change is Reviewable

@iphydf iphydf added this to the v0.2.19 milestone Feb 11, 2024
@iphydf iphydf marked this pull request as ready for review February 11, 2024 16:31
@iphydf iphydf requested a review from a team as a code owner February 11, 2024 16:31
Copy link

codecov bot commented Feb 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.04%. Comparing base (961891d) to head (71cd8de).
Report is 74 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2679      +/-   ##
==========================================
+ Coverage   73.02%   73.04%   +0.01%     
==========================================
  Files         148      148              
  Lines       30482    30482              
==========================================
+ Hits        22261    22265       +4     
+ Misses       8221     8217       -4     

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

other/docker/coverage/run Outdated Show resolved Hide resolved
!other/pkgconfig/*
!other/rpm/*
!so.version
!other/docker/coverage/run_mallocfail
Copy link
Member

Choose a reason for hiding this comment

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

Was this file committed by a mistake? It's supposedly auto-generated by ../sources/run.sh when called by coverage/run.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, it's not a mistake. This file has to be in git because otherwise a default "build this docker file" action (github or render.com) doesn't work the way it should.

Copy link
Member

@nurupo nurupo Feb 11, 2024

Choose a reason for hiding this comment

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

Huh. So, whenever someone changes either the master dockerignore file in other/docker/sources/, or a supplemental one in other/docker/$BUILD/, they would have to run the other/$BUILD/run script(s) to update the concatenated dockerignore file(s) before committing/making a PR?

Maybe we could add a script step to the GitHub CI before the "build this docker file" action that would generate this dockerignore file? For example, move the dockerignore concat section out of other/docker/sources/ru.sh into its own bash script, something like other/docker/source/gen-dockerignore.sh that takes $BUILD as an argument (or a path to $BUILD's directory or something like that) and concats the master dockerignore with the $BUILD's, putting it in $BUILD's directory? That script could be used in CI to generate the dockerignore file before calling the docker actions and will also be called from other/docker/sources/run.sh.

(Also, what is this render.com promotion? How is that relevant to us?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll do that, but it'll take some time for me to find motivation for it, so I'll move this PR to the next milestone.

other/docker/coverage/serve Outdated Show resolved Hide resolved
@pull-request-attention pull-request-attention bot assigned iphydf and nurupo and unassigned iphydf Feb 11, 2024
@iphydf iphydf force-pushed the coverage-sources branch 5 times, most recently from a6518b4 to 72ff594 Compare February 11, 2024 17:38
Use per-dockerfile dockerignore instead of sources for coverage. It's a
bit more duplication (dockerignore is copied out of the sources one),
but it's a bit more efficient and works in more situations (e.g. one
where you can only build 1 dockerfile such as on render.com).
@pull-request-attention pull-request-attention bot assigned iphydf and nurupo and unassigned nurupo and iphydf Feb 11, 2024
@iphydf iphydf modified the milestones: v0.2.19, v0.2.20 Feb 12, 2024
@iphydf iphydf marked this pull request as draft February 17, 2024 21:14
@iphydf iphydf modified the milestones: v0.2.20, v0.2.21 Nov 6, 2024
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.

2 participants