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

formulary: fix loading from taps #16215

Closed
wants to merge 1 commit into from

Conversation

amancevice
Copy link
Contributor

@amancevice amancevice commented Nov 13, 2023

check tapped_name for "/" instead of parsed name

Use tap.core_tap? instead of checking formula name

I'm not 100% sure this is the correct fix but from my reading of the code it seems plausible

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Got a sorbet error running brew typecheck and brew tests but since it's such a small change I hope that's not a deal breaker

@Bo98
Copy link
Member

Bo98 commented Nov 13, 2023

I reckon this should be tap.core_tap? rather than a name check.

check `tapped_name` for "/" instead of parsed `name`
@amancevice
Copy link
Contributor Author

@Bo98 refactored with your suggestion

@Bo98
Copy link
Member

Bo98 commented Nov 13, 2023

Just to check: did you test that it fixes #16213 for you?

@amancevice
Copy link
Contributor Author

Yes, brew reinstall hashicorp/tap/terraform correctly installed the tapped formula with my branch checked out

@Bo98
Copy link
Member

Bo98 commented Nov 13, 2023

Thanks!

@Bo98 Bo98 changed the title fix for 97b4119 formulary: fix loading from taps Nov 13, 2023
@Bo98 Bo98 enabled auto-merge November 13, 2023 16:18
@Rylan12
Copy link
Member

Rylan12 commented Nov 13, 2023

I don't think this is the right solution since tap, in this case, is supposed to refer to the old tap. So comparing to homebrew/core will break tap migrations when homebrew/core isn't installed.

Instead, we need to pass the original tap to the FormulaAPILoader and check here based on the path (which reflects the actual formula file location). I've worked a fix into #16216

@amancevice
Copy link
Contributor Author

sounds good to me, thanks for the quick fix, @Rylan12

@amancevice amancevice closed this Nov 13, 2023
@github-actions github-actions bot added the outdated PR was locked due to age label Dec 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2023
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

3 participants