-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
livecheck migration: create Homebrew::Livecheck #8254
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
livecheck migration: create Homebrew::Livecheck #8254
Conversation
With the latest push, some tests have been added. They are a work in progress ( We have a lot of Formula method calls in livecheck, and as a result I've got a little too many |
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.
Will have more comments to add on this but should give you enough to work on and looks good so far!
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.
A few more comments based on "finishing this off". A few of these are general so please check if there's places I've missed where the same comment may apply. Thanks, great work so far @nandahkrishna!
An update: |
With the latest push, most changes have been addressed. Some points to note:
Looking forward to your reviews @samford and @MikeMcQuaid! |
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.
A few nits but looking good to me. I'm happy to merge this when you're happy with the state of my questions.
Co-authored-by: Sam Ford <1584702+samford@users.noreply.github.com> Co-authored-by: Thierry Moisan <thierry.moisan@gmail.com> Co-authored-by: Dawid Dziurla <dawidd0811@gmail.com> Co-authored-by: Maxim Belkin <maxim.belkin@gmail.com> Co-authored-by: Issy Long <me@issyl0.co.uk> Co-authored-by: Mike McQuaid <mike@mikemcquaid.com> Co-authored-by: Seeker <meaningseeking@protonmail.com>
With the latest push the following changes have been made:
Will make further changes based on your reviews @MikeMcQuaid @samford. Thanks! |
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! Nothing here blocking this from my perspective. 👍🏻 to merge when @samford is.
I'm busy with work at the moment but I'm planning to take a pass at this in the evening (around 5 or 6 hours from now). If this can't wait until then, I imagine it's probably in good enough shape that any issues are likely minor and I could address them in a follow-up PR, if necessary. Otherwise, if this can wait, I'll get to it as soon as I can. |
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've left a few comments but they're mostly straightforward suggestions. The message.presence
issue may require a little discussion but it's something that I would like to sort out before this is merged (as it modifies existing behavior).
Once these latest review items are resolved, I'm good with this PR being merged. The documentation comments could use a little work but I don't want to hold this up, so I'll take care of it in a later PR.
It's very late over here and I'm calling it a night but I'll check back in the morning.
Well, I'm not sure I understand why this (SSL error) is happening with the tests. They all pass locally, and the only difference now compared to a previous CI run that was 🟢, is that I'm now setting the |
The debug output still contains the normal (non-debug) output at the end, so I would think it should still be fine. The test works fine locally, so it must just be that the SSL error prevents livecheck from identifying a version and that in turn results in the expected output not being present. I have no idea why we keep getting an SSL error for any related tests checking GitHub, though. I would be interested to see what happens on CI if we check a non-GitHub URL in the let(:f) do
formula("test") do
desc "Test formula"
homepage "https://brew.sh"
url "https://brew.sh/test-0.0.1.tgz"
head "https://github.com/Homebrew/brew.git"
livecheck do
url "https://formulae.brew.sh/api/formula/readline.json"
regex(/"stable":"(\d+(?:\.\d+)+)"/i)
end
end
end It may be that there's something better to check but this would at least let us know if this is only related to GitHub URLs or if it affects other HTTPS URLs. You would also need to add the previous GitHub URL ( Worst case scenario, it may be that we have to drop the |
That seems to have worked! Thank you Sam. Maybe we should, for whatever reason, avoid GitHub URLs and stick with a brew.sh URL for the network-related tests. |
Hm, I do wonder what the issue is with the GitHub URL. We didn't have any problems checking GitHub URLs on homebrew-livecheck CI, other than rate limiting. The Since this works with a formulae.brew.sh URL, I suppose the question is whether we want to move forward with this or to simply drop the If we end up moving forward with the formulae.brew.sh check, is there any formula that we can pretty much rely on never being deleted in the future? I simply picked |
I picked |
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.
One little change and then I think that's all for me, outside of resolving the open question above.
Co-authored-by: Sam Ford <1584702+samford@users.noreply.github.com>
Great work, thanks again @nandahkrishna! |
brew style
with your changes locally?brew tests
with your changes locally?This PR adds the
Homebrew::Livecheck
module containing livecheck's core logic. PR #8180 was hard to review as it was too large, so it was split up, with the original PR being exclusively to migrate the command.Changes/suggestions yet to handle:
This PR will have to be merged before we can merge #8180.