-
-
Notifications
You must be signed in to change notification settings - Fork 13.2k
granted: install shell wrappers and assumego symlink #248898
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
Conversation
The formula was only installing the granted binary. This left the package non-functional because the assume workflow requires three additional components that weren't being installed. The assume script is a POSIX shell wrapper that calls assumego and exports AWS credentials into the current shell environment. The assume.fish script does the same for fish shell. The assumego binary is accessed via symlink to the granted binary - the code checks argv[0] to determine behavior. Without these components, users couldn't actually assume AWS roles because the credentials wouldn't propagate to their shell session. This adds installation of scripts/assume and scripts/assume.fish from the source tree, and creates the assumego -> granted symlink during the install phase.
|
|
||
| # Install shell wrapper scripts | ||
| bin.install "scripts/assume" | ||
| bin.install "scripts/assume.fish" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| bin.install "scripts/assume.fish" |
bash script alone should be enough, i see no reason to have another script that does the same thing but with .fish extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is required because Granted supports the fish shell (https://docs.commonfate.io/granted/configuration#fish). It is a different script to assume. The regular assume shell script is incompatible with fish, so we need to also include this file with the .fish extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just adding -- during normal use these scripts get sourced into the shell, not executed, so the script doesn't control its interpreter. The script needs to be natively compatible with the shell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they need to be sourced instead of executed, why write them to bin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use granted, and also support fish users using granted. The change above mirrors how granted was installed previously:
Can't speak to why both of these are written to bin, but this does match how the granted authors intended this package to be installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is this:
If we put it in bin, will users be expecting that. Or can we make it easier somehow?
If it needs to be sourced, we can put it in a directory that automatically gets sourced. That seems more user friendly to me.
If it just needs to be in PATH when granted is executed, maybe we can make a wrapper script that does that.
But it's still unclear to me how/if users are expected to interact with assume
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrnorm could you help clarify here? What are users expecting? Are they expecting to call assume? Or are they expecting assume to be sourced when they open their shell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is called by users. However, the first time that Granted runs, it installs an alias in users' shell profile (such as ~/.bashrc), for example:
alias assume="source (brew --prefix)/bin/assume.fish"
so when they call assume they are sourcing the script. The script is not sourced automatically when users open their shell, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so it's only ever called once by users. Because afterwards it will run the alias instead when people do assume. So what if we just make bin/assume a script that sources some/path/assume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrnorm That hardcoded path isn't critical, since we can inreplace it at build. In the event we want to throw it into libexec or something that's what we'd do.
|
Thanks for all the discussion and feedback here @botantony @SMillerDev. I'm going to close this in favor of #249669 |
|
@SMillerDev I suggest reviving this. As this stands, brew does not install granted correctly, causing a decent amount of confusion/frustration for users (who right now are forced to use the vendor tap once they figure out why this package is broken). As for whether or not granted should install a certain way, could this not be deferred to a follow-up PR? Ie. Can we prioritize mitigating user pain first? |
|
I'm all for it. Either revive this PR or make the same modifications in a new one. Then we can discuss how to best help users with the assume script. |
|
thanks @SMillerDev, I’ve re-opened this PR |
|
#249904 -> This might be a clone, but this is definitely breaking my company as well. My vote is to remove the formula as the maintainers seem to prefer running their own tap. |
|
@zwing99 As I mentioned in your issue, which I already closed, we will not be removing this formula. We're having a discussion in here for the best way to handle these requested changes. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This is needed please advise. It is still broken for users of granted. |
|
This was fixed in #255790. |
The formula was only installing the granted binary. This left the package non-functional because the assume workflow requires three additional components that weren't being installed.
The assume script is a POSIX shell wrapper that calls assumego and exports AWS credentials into the current shell environment. The assume.fish script does the same for fish shell. The assumego binary is accessed via symlink to the granted binary - the code checks argv[0] to determine behavior.
Without these components, users couldn't actually assume AWS roles because the credentials wouldn't propagate to their shell session.
This adds installation of scripts/assume and scripts/assume.fish from the source tree, and creates the assumego -> granted symlink during the install phase.
HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where<formula>is the name of the formula you're submitting?brew test <formula>, where<formula>is the name of the formula you're submitting?brew audit --strict <formula>(after doingHOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it passbrew audit --new <formula>?