Add url option to brew.tap #1022

znd4 · 2023-10-26T23:38:49Z

The brew tap command defaults to a (somewhat weird) behavior of looking in a separate homebrew-{repo} repo under the same owner. For understandable reasons, some projects (like the included kpt example), just put the Formula metadata in the main project repo.

Currently, such projects can't be brew taped with pyinfra.

I also took a bit of liberty to refactor the brew tap command to use guard clauses, but I wouldn't mind undoing that.

I'm not sure where the coverage drop is coming from; trying to nail that down

znd4 · 2023-10-26T23:40:07Z

.editorconfig

@@ -0,0 +1,3 @@
+[*.json]


I don't mind removing this, or filling it out more (e.g. with *.py or *.toml configuration)

Let's leave it as-is, editorconfig is neat!

znd4 · 2023-10-26T23:43:00Z

pyinfra/operations/brew.py

-            taps.remove(src)
-        else:
-            host.noop("tap {0} does not exist".format(src))
+        cmd = "brew tap {0}".format(src)


let me know if this guard clause refactoring isn't appreciated -- no hard feelings about reverting to just the url change

Much appreciated, good improvement 👍

znd4 · 2023-10-27T02:10:56Z

pyinfra/operations/brew.py

@@ -159,12 +159,13 @@ def casks(


 @operation
-def tap(src, present=True):
+def tap(src, present=True, url=None):


We might be able to make src optional if url is present?

Alternative: src could accept an URL? apt.key does this:

pyinfra/pyinfra/operations/apt.py

Lines 75 to 80 in a280e17

# If URL, wget the key to stdout and pipe into apt-key, because the "adv"

# apt-key passes to gpg which doesn't always support https!

if urlparse(src).scheme:

yield "(wget -O - {0} || curl -sSLf {0}) | apt-key add -".format(src)

else:

yield "apt-key add {0}".format(src)

I'm actually not 100% this is the best route (too magic?), would like your opinion!

xD I have gotten a bit disillusioned with excess magic since spending time in Golang-land, but the bigger issue with that is that it wouldn't be able to support the full scope of brew tap [user/repo] [URL].

Based on this search, it seems like the URL argument is usually either

brew tap user/projectname https://github.com/user/projectname (the behavior we'd support by parsing src as a URL and looking at the last two path components)
or

brew tap user/projectname https://github.com/user/homebrew-projectname (In this case, I'm pretty sure the url is redundant)

However, sometimes people add .git at the end of the URL. We could add a special case for that, but there's a less tractable issue:

A (small) percentage of documented uses of brew tap don't end with owner/repo at all, for example:

brew tap team23/b5 https://git.team23.de/build/homebrew-b5.git

Since our implementation would calculate the tap name as build/homebrew-b5 (or build/b5), it seems like a painful failure mode.

I suppose we could "do both" --

# valid brew.tap(\"user/project\") brew.tap(\"https://github.com/user/project\") brew.tap(\"owner/project\", \"https://git.owner.com/foo/bar.git\") # throw errors? brew.tap(\"https://git.owner.com/foo\") # path is too short brew.tap(\"https://git.owner.com/foo/bar.git\")

I suppose we could "do both":

valid

brew.tap(\"user/project\") brew.tap(\"https://github.com/user/project\") # this feels really nice brew.tap(\"owner/project\", url=\"https://git.owner.com/foo/bar.git\")

unsure

could be brew tap owner/project https://github.com/owner/project.git

brew.tap(\"https://github.com/owner/project.git\")

# or \"brew tap owner/project.git https://github.com/owner/project.git\" brew.tap(\"https://git.owner.com/foo/bar\")

invalid

brew.tap(\"https://git.owner.com/foo\") # path is too short

I suppose we could "do both":

valid

brew.tap(\"user/project\") brew.tap(\"https://github.com/user/project\") # this feels really nice brew.tap(\"owner/project\", url=\"https://git.owner.com/foo/bar.git\")

unsure

brew.tap(\"https://github.com/owner/project.git\")

could be brew tap owner/project https://github.com/owner/project.git

# or \"brew tap owner/project.git https://github.com/owner/project.git\" brew.tap(\"https://git.owner.com/foo/bar\")

invalid

brew.tap(\"https://git.owner.com/foo\") # path is too short

I suppose we could "do both":

valid

brew.tap(\"user/project\") brew.tap(\"https://github.com/user/project\") # this feels really nice brew.tap(\"owner/project\", url=\"https://git.owner.com/foo/bar.git\")

unsure

brew.tap(\"https://github.com/owner/project.git\")

could be brew tap owner/project https://github.com/owner/project.git
or "brew tap owner/project.git https://github.com/owner/project.git\"

brew.tap(\"https://git.owner.com/foo/bar\")

should we assume foo is the owner for non-github / bitbucket / ... URLs? It seems clunky to have an allowlist of known git hosts, but otherwise this is still a bit of a footgun

invalid

brew.tap(\"https://git.owner.com/foo\") # path is too short

I think github went wild with some edits there by the looks of it!

I totally agree with the magic thing (funny - also Go that has altered my perspective on such things), let's keep the explicit URL and avoid the magic/complexity.

SO I think the only change here is to make the src optional!

Fizzadar

@znd4 firstly - apologies for taking way too long to review this, it has been a crazy few months and I have fallen way behind. I very much appreciate the PR and it looks great! Couple of comments to discuss but this looks good to go.

Fizzadar · 2023-11-25T20:17:19Z

.editorconfig

@@ -0,0 +1,3 @@
+[*.json]


Let's leave it as-is, editorconfig is neat!

Fizzadar · 2023-11-25T20:22:01Z

pyinfra/operations/brew.py

@@ -159,12 +159,13 @@ def casks(


 @operation
-def tap(src, present=True):
+def tap(src, present=True, url=None):


Alternative: src could accept an URL? apt.key does this:

pyinfra/pyinfra/operations/apt.py

Lines 75 to 80 in a280e17

# If URL, wget the key to stdout and pipe into apt-key, because the "adv"

# apt-key passes to gpg which doesn't always support https!

if urlparse(src).scheme:

yield "(wget -O - {0} || curl -sSLf {0}) | apt-key add -".format(src)

else:

yield "apt-key add {0}".format(src)

I'm actually not 100% this is the best route (too magic?), would like your opinion!

Fizzadar · 2023-11-25T20:22:43Z

pyinfra/operations/brew.py

-            taps.remove(src)
-        else:
-            host.noop("tap {0} does not exist".format(src))
+        cmd = "brew tap {0}".format(src)


Much appreciated, good improvement 👍

znd4 · 2023-11-29T03:17:03Z

apologies for taking way too long to review this

no worries! Fixing one issue on an ~8 star repo of mine took up most of a weekend, so I can only imagine how much is required to maintain a project this lively.

znd4 · 2023-12-19T20:48:14Z

sors, I was slammed at work recently.

Decided to remove the pre-commit config as soon as I put it in, because it should include changes to CONTRIBUTING.md, and potentially some CI changes, so it should be a standalone PR (assuming you even want to add it)

Fizzadar

Awesome stuff, thank you for putting this together @znd4!

Yes please for a pre-commit PR if you have time 🙏

first attempt

a5e2b42

znd4 commented Oct 26, 2023

View reviewed changes

remove and ignore ropeproect

c57ba48

znd4 commented Oct 26, 2023

View reviewed changes

znd4 marked this pull request as ready for review October 26, 2023 23:43

another guard clause refactor; add another unit test

ddd4112

znd4 commented Oct 27, 2023

View reviewed changes

Fizzadar reviewed Nov 25, 2023

View reviewed changes

znd4 and others added 4 commits November 30, 2023 09:56

make src optional for brew.tap

b028836

Merge branch '2.x' into add-url-option-to-brew.tap

69b0b26

isort and black brew.py, and add pre-commit conig

aa1b023

remove pre-commit config

371afbe

Fizzadar approved these changes Jan 12, 2024

View reviewed changes

Fizzadar merged commit a773e32 into pyinfra-dev:2.x Jan 12, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add url option to brew.tap #1022

Add url option to brew.tap #1022

znd4 commented Oct 26, 2023 •

edited

Loading

znd4 Oct 26, 2023

Fizzadar Nov 25, 2023

znd4 Oct 26, 2023

Fizzadar Nov 25, 2023

znd4 Oct 27, 2023

Fizzadar Nov 25, 2023

znd4 Nov 29, 2023

znd4 Nov 29, 2023

znd4 Nov 29, 2023

znd4 Nov 29, 2023

znd4 Nov 29, 2023

znd4 Nov 29, 2023

Fizzadar Nov 29, 2023

Fizzadar Nov 29, 2023

Fizzadar left a comment

Fizzadar Nov 25, 2023

Fizzadar Nov 25, 2023

Fizzadar Nov 25, 2023

znd4 commented Nov 29, 2023

znd4 commented Dec 19, 2023

Fizzadar left a comment

	# If URL, wget the key to stdout and pipe into apt-key, because the "adv"
	# apt-key passes to gpg which doesn't always support https!
	if urlparse(src).scheme:
	yield "(wget -O - {0} \|\| curl -sSLf {0}) \| apt-key add -".format(src)
	else:
	yield "apt-key add {0}".format(src)

Add url option to brew.tap #1022

Add url option to brew.tap #1022

Conversation

znd4 commented Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valid

unsure

invalid

Choose a reason for hiding this comment

valid

unsure

invalid

Choose a reason for hiding this comment

valid

unsure

invalid

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fizzadar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znd4 commented Nov 29, 2023

znd4 commented Dec 19, 2023

Fizzadar left a comment

Choose a reason for hiding this comment

znd4 commented Oct 26, 2023 •

edited

Loading