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

Improve macOS homebrew support for cask packages #53756

Closed

Conversation

cdalvaro
Copy link
Contributor

@cdalvaro cdalvaro commented Jul 9, 2019

What does this PR do?

When listing Homebrew packages, cask packages do not have the right namespace when they are from a tap different from the default one.

This PR fixes this issue recovering the right namespace for each cask package.

Previous Behavior

local:
----------
          ID: FiraCode Nerd Font
    Function: pkg.installed
        Name: homebrew/cask-fonts/font-firacode-nerd-font
      Result: False
     Comment: The following packages failed to install/update: homebrew/cask-fonts/font-firacode-nerd-font
     Started: 17:59:08.334054
    Duration: 14324.087 ms
     Changes:

New Behavior

local:
----------
          ID: FiraCode Nerd Font
    Function: pkg.installed
        Name: homebrew/cask-fonts/font-firacode-nerd-font
      Result: True
     Comment: All specified packages are already installed
     Started: 18:56:36.057920
    Duration: 48438.613 ms
     Changes:

Tests written?

Yes

Commits signed with GPG?

Yes

@cdalvaro
Copy link
Contributor Author

cdalvaro commented Jul 10, 2019

This PR breaks backward compatibility in terms that all salt recipes that install brew-cask packages with the namespace caskroom/cask won’t be verified properly although the installation has been performed successfully.

Those recipes must be changed from caskroom/cask/foo to homebrew/cask/foo in order to be verified again.

I tried sometime ago to get the same behaviour for brew-cask as with brew info --json=v1 --installed.
The request was intended to return the whole namespace for a brew-cask package, but the PR Homebrew/homebrew-cask#56837 was discarded.

So I have used a second approach to get the correct namespace for each package.

As mention in the official documentation, you can make your custom @Homebrew tap by creating a GitHub repository that begins by homebrew- so your tap would be something like username/homebrew-something.

This behaviour also applies to official taps, so this is the reason why I have change the old namespace (caskroom/cask) to the new one (homebrew/cask).

@cdalvaro cdalvaro changed the title WIP: Improve macOS homebrew support for cask packages Improve macOS homebrew support for cask packages Jul 18, 2019
@cdalvaro
Copy link
Contributor Author

This PR is ready for review 🔍

@cdalvaro cdalvaro changed the base branch from develop to 2019.2.1 August 7, 2019 05:32
@cdalvaro cdalvaro force-pushed the improve_macOS_hombrew_cask_support branch from ed584e6 to aace02a Compare August 7, 2019 05:58
@cdalvaro cdalvaro force-pushed the improve_macOS_hombrew_cask_support branch from aace02a to b570a0e Compare August 7, 2019 18:01
@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 14, 2019

It concerns me that we are making a backwards compatible change here. Normally if there is a backwards compatible change we have to follow the deprecating code docs here: https://docs.saltstack.com/en/latest/topics/development/deprecations.html and only deprecate it in two releases in the future.

Is there a way we can allow both recipes going forward?

also @weswhet can you review here as well?

@Ch3LL Ch3LL self-requested a review August 14, 2019 14:25
@weswhet
Copy link
Contributor

weswhet commented Aug 14, 2019

I’ll have a look when I get in the office. Thanks for the ping.

@cdalvaro
Copy link
Contributor Author

cdalvaro commented Aug 14, 2019

It concerns me that we are making a backwards compatible change here. Normally if there is a backwards compatible change we have to follow the deprecating code docs here: https://docs.saltstack.com/en/latest/topics/development/deprecations.html and only deprecate it in two releases in the future.

Is there a way we can allow both recipes going forward?

also @weswhet can you review here as well?

Thank you for your response @Ch3LL

One workaround to maintain backward compatibility could be to check in methods install, remove and upgrade if the given package (or packages) begins with caskroom/cask, and in that case, replace it with homebrew/cask.

if name.startswith('caskroom/cask/'):
  salt.utils.versions.warn_until(
            'Neon',
            'The \'caskroom/cask/\' namespace for brew-cask packages'
            'is deprecated. Use \'homebrew/cask/\' instead.'
        )
  name = name.replace("caskroom/cask/", "homebrew/cask/")

If this change looks good to you I can make it. 😀

@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 14, 2019

would like to get @weswhet 's input on that approach as he is more familiar with these modules

@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 14, 2019

Also forgot to mention. We are very close to tagging v2019.2.1 and only merging in test fixes and pre-defined PRs. Are you okay moving this to the 2019.2 branch?

@cdalvaro
Copy link
Contributor Author

Also forgot to mention. We are very close to tagging v2019.2.1 and only merging in test fixes and pre-defined PRs. Are you okay moving this to the 2019.2 branch?

No problem!

Is this something that depends on me?

@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 14, 2019

It would be easier if you do it. You can try to change the branch in Github but there is likely to be some conflicts. Its probably easiest to just close here and re-submit against the 2019.2 branch. If you can't do it i can though.

@cdalvaro
Copy link
Contributor Author

It would be easier if you do it. You can try to change the branch in Github but there is likely to be some conflicts. Its probably easiest to just close here and re-submit against the 2019.2 branch. If you can't do it i can though.

No problem! I'll do the change tomorrow!

@cdalvaro
Copy link
Contributor Author

I have created PR #54216 as requested by @Ch3LL to merge my changes into branch 2019.2

I have added backward compatibility for old recipes and added the deprecation warning too.

I close this PR now in favour of #54216

@cdalvaro cdalvaro closed this Aug 15, 2019
@cdalvaro cdalvaro deleted the improve_macOS_hombrew_cask_support branch September 26, 2019 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants