Skip to content
This repository has been archived by the owner on Dec 2, 2020. It is now read-only.

Updates brewcask_root location. #48

Merged
merged 1 commit into from
Jan 8, 2017
Merged

Conversation

typhonius
Copy link
Contributor

Inline with #47 and Homebrew/homebrew-cask#21901 the default brewcask_root location should be changed so boxen doesn't throw warnings.

@jacobbednarz
Copy link
Member

This seems reasonable and can still be overridden in facts.d/example.yaml if it doesn't suit.

@jacobbednarz jacobbednarz merged commit d9fae78 into boxen:master Jan 8, 2017
@jacobbednarz
Copy link
Member

released via 0.0.8

@tommeier
Copy link
Member

This breaks things for me when bumping up the version (with latest boxen):

Error: Duplicate declaration: File[/usr/local] is already declared in file /opt/boxen/repo/shared/homebrew/manifests/init.pp:53; cannot redeclare at /opt/boxen/repo/shared/brewcask/manifests/init.pp:17 on node xxx
Error: Duplicate declaration: File[/usr/local] is already declared in file /opt/boxen/repo/shared/homebrew/manifests/init.pp:53; cannot redeclare at /opt/boxen/repo/shared/brewcask/manifests/init.pp:17 on node xxx

@tommeier
Copy link
Member

With the following combination from the Puppetfile:

github "brewcask",    "0.0.8"
github "homebrew",    "2.1.0"

@typhonius
Copy link
Contributor Author

typhonius commented Jan 11, 2017

This is because the following in manifests/init.pp also evaluates to /usr/local:

file { $cask_home:
    ensure => directory
}

We could potentially do away with this declaration since this file explicitly requires homebrew however this would then fail for users who specify a different brewcask_root. Something else that could be done is to wrap the above in the following but I'm not sure how good a practice this is considered.:

if ! defined(File["${cask_home}"]) {
}

@tommeier
Copy link
Member

alternatively wrap either/both in :

if ! defined(File[$cask_home]) {
  file { $cask_home:
      ensure => directory
  }
}

@tommeier
Copy link
Member

I do that pattern reguarly for sub items, the only real risk is if other declarations make more explicit options, that may or may not be what you want. For this case, as its just ensuring a directory in both base homebrew and brewcask I would think its OK.

@jacobbednarz
Copy link
Member

We could potentially do away with this declaration since this file explicitly requires homebrew however this would then fail for users who specify a different brewcask_root.

I think we need to support the combination of the following:

  • Homebrew at /usr/local OR /opt/boxen/homebrew
  • Brewcask at /usr/local OR /opt/homebrew-cask/Caskroom

One approach we could try is check if homebrew lives at /usr/local and if it does append Caskroom to the path. Should homebrew live anywhere else, set the path to /opt/homebrew-cask/Caskroom.

I'd like to avoid !defined(File[$cask_home]) style checks because it introduces another conditional into the manifest that is difficult to test.

@jacobbednarz
Copy link
Member

Thanks for staying in the loop for this one @tommeier - I appreciate it!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants