[FEATURE] Introduces homebrew task #67
Conversation
mlhamel
force-pushed
the
introduces-homebrew-task
branch
from
d98f0c2
to
7e105c5
Compare
|
😍 |
pkg/env/env.go
Outdated
| // Os returns the current running operating system | ||
| func (e *Env) Os() string { | ||
| return runtime.GOOS | ||
| } |
There was a problem hiding this comment.
Why do we need this indirection? Why not just call runtime.GOOS directly?
pkg/helpers/homebrew.go
Outdated
| func NewHomebrewWithPrefix(prefix string) *Homebrew { | ||
| if !utils.PathExists(filepath.Join(prefix, "Cellar")) { | ||
|
|
||
| path := strings.Split(env.NewFromOS().Get("PATH"), ":") |
There was a problem hiding this comment.
We could probably update the Env api to make getAndSplitPath() public.
Renaming the 2 symmetric methods could be a good idea: GetPathParts()/SetPathParts()
pkg/helpers/homebrew.go
Outdated
|
|
||
| if len(path) > 0 && utils.PathExists(path[0]) { | ||
| prefix = filepath.Dir(path[0]) | ||
| } |
There was a problem hiding this comment.
I don't get this. Are we making an assumption on what the first PATH member is?
pkg/helpers/homebrew.go
Outdated
| return &Homebrew{prefix: prefix} | ||
| } | ||
|
|
||
| func pathToPackage(filename string) string { |
There was a problem hiding this comment.
Just a thought: maybe the helpers should get their own package (pkg/helpers/homebrew) to scope function like this pathToPackage().
Otherwise we should either namespace it with homebrew or attach it to Homebrew
There was a problem hiding this comment.
Good idea the sub-package cause i fear this might end-up has a mess of name collisions
pkg/helpers/homebrew.go
Outdated
| func (h *Homebrew) PackageIsInCaskroom(pkg string) bool { | ||
| path := "/opt/homebrew-cask/Caskroom" | ||
|
|
||
| if !utils.PathExists("/opt/homebrew-cask/Caskroom") { |
pkg/helpers/homebrew.go
Outdated
| path := filepath.Join(h.prefix, "Cellar") | ||
|
|
||
| path = filepath.Join(path, pkg) | ||
| return utils.PathExists(path) |
There was a problem hiding this comment.
Nit: return utils.PathExists(filepath.Join(h.prefix, "Cellar", pkg))
pkg/helpers/homebrew_test.go
Outdated
| ) | ||
|
|
||
| func TestPackageIsInCellar(t *testing.T) { | ||
| prefix, err := ioutil.TempDir("/tmp", "dad-brew") |
There was a problem hiding this comment.
- is there a reason to specify the tmpdir?
ioutil.TempDir("", ...) - we should clean this folder up after the test.
But all that is annoying, let's use Filet (it's already used in this project):
https://github.com/Flaque/filet#creating-temporary-directories
mlhamel
force-pushed
the
introduces-homebrew-task
branch
from
7e105c5
to
0974600
Compare
|
ping @pior I've updated the homebrew feature Just a few notes, I've decided not installing the formulas from the helper as we've discussed but directly in the TaskAction cause we might needs to refactor a bit the task api for that and I would prefer doing this in a separated PR. |
pkg/env/env.go
Outdated
| @@ -62,30 +62,32 @@ func (e *Env) Environ() (vars []string) { | |||
| return vars | |||
| } | |||
|
|
|||
| func (e *Env) getAndSplitPath() []string { | |||
| // GetPathParts gets paths in PATH environment variable | |||
| func (e *Env) GetPathParts() []string { | |||
There was a problem hiding this comment.
Nit: do we need this to be public in the end?
pkg/helpers/homebrew.go
Outdated
| return h.cellar.IsInstalled(path) || h.caskroom.IsInstalled(path) | ||
| } | ||
|
|
||
| func buildFormulaPath(filename string) string { |
There was a problem hiding this comment.
The logic doesn't seem so obvious, I feel like we should explain what happens here.
pkg/helpers/homebrew.go
Outdated
| } | ||
|
|
||
| // IsInstalled returns true if formulua was installed in Caskrook | ||
| func (c *caskroom) IsInstalled(formula string) bool { |
There was a problem hiding this comment.
Those methods (also on cellar) looks like they should be private.
|
|
||
| h := NewHomebrewWithPrefix(prefix) | ||
|
|
||
| require.Truef(t, h.IsInstalled("curl"), "Curl is properly installed in Cellar %s", cellarPath) |
There was a problem hiding this comment.
I missed those require functions, nice!
pkg/tasks/homebrew.go
Outdated
| brew := helpers.NewHomebrew() | ||
|
|
||
| if !brew.IsInstalled(b.formula) { | ||
| err := command(ctx, "brew", "install", b.formula).AddOutputFilter("already satisfied").Run() |
There was a problem hiding this comment.
Does this AddOutputFilter make sense with brew?
There was a problem hiding this comment.
Yeah not really cause anyway we should just display those informations
pkg/tasks/homebrew.go
Outdated
| func (b *brewInstall) run(ctx *context) error { | ||
| brew := helpers.NewHomebrew() | ||
|
|
||
| if !brew.IsInstalled(b.formula) { |
There was a problem hiding this comment.
This condition should be in the needed method, so the TaskRunner knows whether this action should be run or not.
Resolves #65