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

livecheck migration: create Homebrew::Livecheck #8254

Merged
merged 5 commits into from Aug 31, 2020
Merged

livecheck migration: create Homebrew::Livecheck #8254

merged 5 commits into from Aug 31, 2020

Conversation

nandahkrishna
Copy link
Member

@nandahkrishna nandahkrishna commented Aug 8, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run 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:

  • I'm working on the tests and am nearly done with them, I'll push them soon.

This PR will have to be merged before we can merge #8180.

@nandahkrishna
Copy link
Member Author

With the latest push, some tests have been added. They are a work in progress (style will fail as well) and I was wondering whether using a double as a substitute for a Formula was okay.

We have a lot of Formula method calls in livecheck, and as a result I've got a little too many allows, and some for method chaining as well. The alternative I'm considering is using :integration_test so that I can setup a test formula (just as in the dev-cmd PR's test). What do you think @MikeMcQuaid?

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a 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!

Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a 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!

Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/livecheck/livecheck_spec.rb Outdated Show resolved Hide resolved
@nandahkrishna
Copy link
Member Author

An update:
Most changes have been completed, it's just the tests and trying to combine code from audit or notability left. Should be pushing them soon.

@nandahkrishna
Copy link
Member Author

With the latest push, most changes have been addressed. Some points to note:

  • This PR depends on livecheck migration: add strategies #8255 and the final livecheck_formulae test will be added after that PR is merged.
  • Would we require more tests for other methods or would that be too much?
  • I don't think we can do away with the GITHUB_SPECIAL_CASES, so if there's anything we have to do to ensure the formulae in that list still exist, I'll create a PR for that (maybe along with the RuboCops and audits).

Looking forward to your reviews @samford and @MikeMcQuaid!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a 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.

Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
nandahkrishna and others added 2 commits August 27, 2020 21:26
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>
@nandahkrishna
Copy link
Member Author

With the latest push the following changes have been made:

  • Some documentation that definitely needs to be improved.
  • Tests for preprocess_url and livecheck_formulae (both of which needed one unfrozen Formula URL String).
  • Changed the usage of formula.stable? to formula.head? in most cases.

Will make further changes based on your reviews @MikeMcQuaid @samford. Thanks!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a 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.

Library/Homebrew/test/.rubocop_todo.yml Show resolved Hide resolved
@samford
Copy link
Member

samford commented Aug 27, 2020

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.

Copy link
Member

@samford samford left a 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.

Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/livecheck/livecheck_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/livecheck/livecheck_spec.rb Outdated Show resolved Hide resolved
@nandahkrishna
Copy link
Member Author

nandahkrishna commented Aug 28, 2020

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 debug? value to true in an attempt to increase coverage. I'll take a look at this in more detail but do let me know if there's something I'm doing wrong.

@samford
Copy link
Member

samford commented Aug 28, 2020

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 livecheck_formulae test. Just as a quick example, we could check some formulae.brew.sh JSON data:

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 (https://github.s3.amazonaws.com/Homebrew/brew/releases/latest) as a separate variable and update the preprocess_url test to use that, otherwise that test will be unhappy.

Worst case scenario, it may be that we have to drop the livecheck_formulae test for now and address it sometime after GSoC with the other network-related tests (like we did with the PageMatch#find_versions test in the strategies PR).

@nandahkrishna
Copy link
Member Author

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.

@samford
Copy link
Member

samford commented Aug 28, 2020

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 tag_info test in the Git strategy also checks a GitHub URL and that seemed to work, so it's a bit baffling.

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 livecheck_formulae test in favor of working on it later with the other network-related tests. I'm fine either way, so I'll leave it up to @MikeMcQuaid.

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 readline because it's the most popular non-versioned formula.

@samford samford dismissed their stale review August 28, 2020 21:32

Requested changes have been addressed

@nandahkrishna
Copy link
Member Author

I picked ruby because pretty much everything in Homebrew is done with Ruby, and though we use the system Ruby I couldn't see a future where we would remove the ruby formula. Of course, if there's a better alternative I'm okay with changing it.

Copy link
Member

@samford samford left a 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.

Library/Homebrew/test/livecheck/livecheck_spec.rb Outdated Show resolved Hide resolved
Co-authored-by: Sam Ford <1584702+samford@users.noreply.github.com>
@MikeMcQuaid MikeMcQuaid merged commit 30e177f into Homebrew:master Aug 31, 2020
@MikeMcQuaid
Copy link
Member

Great work, thanks again @nandahkrishna!

@nandahkrishna nandahkrishna deleted the migrate-livecheck-module branch August 31, 2020 07:11
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 14, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants