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

Improve dependencies check against :control_branch #116

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

ananace
Copy link
Member

@ananace ananace commented Jan 8, 2025

With this, ra10ke will no longer generate false positives and excessive log spam on versions of the R10k gem >= 3.10.0

It also improves performance and removes unnecessary load by completely skipping version checks for modules using control branch, since that will always be a no-op anyway, and the check would cause each repo to be pulled twice just to do said no-op. (First by R10k's cache verifying that the branch exists, then followed by ra10ke's Git.ls_remote call to check that it really is a branch)

As it turns out, the threaded checking caused git to run into the subshell limit on our r10k repo if the repository host was slow to respond.

@ananace ananace changed the title Improve deprecation check against :control_branch Improve dependencies check against :control_branch Jan 8, 2025
Will no longer generate false positivies and excessive log spam on R10k
>= 3.10.0
Also skips checking version updates for modules using control branch,
since that will always be a no-op anyway
@ananace ananace force-pushed the improved-controlbranch branch from c78cc10 to f1dfa6c Compare January 8, 2025 15:42
@bastelfreak
Copy link
Member

Thanks for the PR! What kind of false positives did you get? And which errors? Is that something we should document in the changelog?

@ananace
Copy link
Member Author

ananace commented Jan 8, 2025

$ bundle exec rake r10k:dependencies |& tee error.log
...
$ wc -l error.log
1647 error.log

We're running this in CI, so the false positive is mainly that the check errors out completely if there's a :control_branch module, as r10k will now refuse to validate its ref.
It helpfully throws a warning message in the log for each such module, before then also generating an error for each of the threads.

E.g.

vendor/ruby/3.2.0/gems/r10k-5.0.0/lib/r10k/module/git.rb:167:in `validate_ref': Unable to manage Puppetfile content 'aaa_clients': Could not resolve control repo branch and no default provided. r10k no longer hardcodes 'master' as the default ref. Consider setting a ref per module in the Puppetfile or setting git:default_ref in your r10k config. (ArgumentError)

Setting an actual R10K::Environment object was enough to get past that error/warning spam, but then we ran into the threading issue - which was causing EPIPE errors as git was failing to spin up porcelain subshells for all our 328 control_branch modules.

 

Not sure exactly what to put in the changelog though, it's a general performance improvement if you're using control_branch, but also an error fix for r10k gem versions 3.10.0 and newer.

@bastelfreak
Copy link
Member

I raised #117 to have the bugfix in the changelog

@bastelfreak bastelfreak merged commit 78714ca into voxpupuli:master Jan 8, 2025
5 checks passed
@ananace ananace deleted the improved-controlbranch branch January 8, 2025 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants