-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
bump-formula-pr: autobump livecheck-defined resources #20700
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
Conversation
This expands prior logic for `:parent` resources to also allow autobumping livecheck-defined resources. Only works for some standard formats so can't deal with on_system blocks within resources, comments, etc. yet.
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good when 🟢 and @samford is 👍🏻
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Apologies for the delay, I'll try to review this tonight. The notification ended up being buried in my inbox until stalebot reminded me. |
samford
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't manually tested this but allowing bump-formula-pr to update any resources with a livecheck block (beyond those using formula :parent) makes sense to me as the next step in gradually expanding support. Figuring out if a new version is appropriate for the formula's software is the hard part when it comes to updating resources but resource livecheck blocks should be accounting for that, at least in theory. I haven't checked the existing resource livecheck blocks recently but it would be a good idea to confirm that's still true before merging this (I can probably take a look tomorrow when I have a moment).
My review comments here only ended up being stylistic tweaks, as it looks reasonable otherwise (granted, it's late and my brain is tired, so take that with a grain of salt 😆).
| new_version = formula_version | ||
|
|
||
| if resource.livecheck.formula != :parent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| new_version = formula_version | |
| if resource.livecheck.formula != :parent | |
| if resource.livecheck.formula == :parent | |
| new_version = formula_version | |
| else |
We only want to set new_version to formula_version when the livecheck block uses formula :parent, so an if/else setup may make this more explicit. The existing logic works but I think this suggestion is easier to follow.
| verbose: args.verbose?) | ||
| new_version = version_info.dig(:version, :latest) | ||
| odie "Resource \"#{resource.name}\" livecheck failed to get latest version!" if new_version.blank? | ||
| next if old_version == new_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| next if old_version == new_version | |
| next if new_version == old_version |
This may just be me but with comparisons, I prefer to put the variable that I'm interested in first. In this case, this suggestion would read as, "Don't proceed if the new version is the same as the old version" and I think that's easy to follow and makes it clear what we're interested in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, can update to this.
Unrelated but GitHub batch apply is suddenly not working so may have to apply each suggestion individually.
| new_version | ||
| elsif (old_tag = resource.specs[:tag]) | ||
| new_tag = old_tag.gsub(old_version, new_version) | ||
| odie "Resource \"#{resource.name}\" tag could not be updated!" if old_tag == new_tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| odie "Resource \"#{resource.name}\" tag could not be updated!" if old_tag == new_tag | |
| odie "Resource \"#{resource.name}\" tag could not be updated!" if new_tag == old_tag |
Same here, if you agree with the previous comment.
| EOS | ||
| end | ||
| # Allow skipping different tags on same revision | ||
| next if old_revision == new_revision |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| next if old_revision == new_revision | |
| next if new_revision == old_revision |
| ^(?<ws>[ ]+)resource\ "#{resource.name}"\ do\s+ | ||
| url\ "#{Regexp.escape(old_url)}",\s+ | ||
| (using:[ ]+.*,\s+)? | ||
| (tag:[ ]+.*,\s+)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ^(?<ws>[ ]+)resource\ "#{resource.name}"\ do\s+ | |
| url\ "#{Regexp.escape(old_url)}",\s+ | |
| (using:[ ]+.*,\s+)? | |
| (tag:[ ]+.*,\s+)? | |
| ^(?<ws> +)resource\ "#{resource.name}"\ do\s+ | |
| url\ "#{Regexp.escape(old_url)}",\s+ | |
| (using: +.*,\s+)? | |
| (tag: +.*,\s+)? |
Unless I'm overlooking something, I believe you should be able to use + here instead of [ ]+. The previous regex was using [ ]+ because the space was at the very start of the line but that's not the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extended mode (x) so either need \ or [ ]. Could switch to former as it is used it in other parts of regexp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think we should add more regexp for this when we have AST logic already. They will be brittle and error-prone. Ideally we'd move away from the existing regexp in bump* too because they predate our vendored AST logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think we should add more regexp for this when we have
ASTlogic already. They will be brittle and error-prone. Ideally we'd move away from the existing regexp inbump*too because they predate our vendoredASTlogic.
Sounds reasonable.
Won't have time to look into this so will close PR for now in case anyone else wants to work on it.
| new_resource_block += "\\k<ws> version \"#{new_version}\"\n" if forced_version | ||
| else | ||
| new_url = update_url(old_url, old_version, new_version) | ||
| if old_url == new_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if old_url == new_url | |
| if new_url == old_url |
|
|
||
| # Capture old_url to avoid possible HEAD resources | ||
| inreplace_regex = / | ||
| ^(?<ws>[ ]+)resource\ "#{resource.name}"\ do\s+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ^(?<ws>[ ]+)resource\ "#{resource.name}"\ do\s+ | |
| ^(?<ws> +)resource\ "#{resource.name}"\ do\s+ |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulling ✅, sorry.
| ^(?<ws>[ ]+)resource\ "#{resource.name}"\ do\s+ | ||
| url\ "#{Regexp.escape(old_url)}",\s+ | ||
| (using:[ ]+.*,\s+)? | ||
| (tag:[ ]+.*,\s+)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think we should add more regexp for this when we have AST logic already. They will be brittle and error-prone. Ideally we'd move away from the existing regexp in bump* too because they predate our vendored AST logic.
|
@cho-m Ok, fine to merge this approach as-is as long as we create at least a |
|
I won't have time to finish up PR for a couple weeks. I can update PR when I am available assuming no one has superseded it. |
|
@cho-m Ok, closing until then. When you reopen can we try the AST approach please. |
This expands prior logic for
:parentresources to also allow autobumping livecheck-defined resources.Only works for some standard formats so can't deal with on_system blocks within resources, comments, etc. yet.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?