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

clone-job: Fix accidentally skipping asset downloads #4360

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

Martchus
Copy link
Contributor

When skipping an asset download, the clone-job script skipped all remaining
assets of the same type as well. This is wrong, e.g. if the uefi-vars asset
is skipped before the actual HDD image is considered the actual HDD image
would be skipped as well. With this change only the uefi-vars asset would
be skipped.

Note that this problem was made worse by e0bb6cb which added one more
case to skip assets.

@BillAnastasiadis
Copy link
Contributor

LGTM. The commit message fail probably comes from the checker not recognizing the tag as a tag, due to the - present. Maybe we should fix that in the checker.

@Martchus
Copy link
Contributor Author

Yes, I think we should fix it in the checker. Being able to use a prefix like this is quite useful. Especially if one creates multiple commits which are all about the same context it is much clearer to specify the context as prefix instead somehow make it clear within each individual commit message.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Can you fix the git commit message regex directly? Should be easy for you. Also the full stack test fails. Please look into that as well, bump retries to 1k or something more smart :)

@perlpunk
Copy link
Contributor

Also the full stack test fails. Please look into that as well, bump retries to 1k or something more smart :)

Problem is that the retries aren't even used, it's stopped in the third one because a CircleCI limit itself is reached

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #4360 (300d1e0) into master (0668021) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4360   +/-   ##
=======================================
  Coverage   97.94%   97.94%           
=======================================
  Files         371      371           
  Lines       33705    33705           
=======================================
  Hits        33012    33012           
  Misses        693      693           
Impacted Files Coverage Δ
t/35-script_clone_job.t 100.00% <ø> (ø)
lib/OpenQA/Script/CloneJob.pm 96.22% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0668021...300d1e0. Read the comment docs.

When skipping an asset download, the clone-job script skipped all remaining
assets of the same type as well. This is wrong, e.g. if the uefi-vars asset
is skipped before the actual HDD image is considered the actual HDD image
would be skipped as well. With this change only the uefi-vars asset would
be skipped.

Note that this problem was made worse by e0bb6cb which added one more
case to skip assets.
@mergify mergify bot merged commit ba3ae18 into os-autoinst:master Nov 16, 2021
@Martchus Martchus deleted the fix-clone-job branch November 16, 2021 17:10
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.

5 participants