-
-
Notifications
You must be signed in to change notification settings - Fork 121
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
Start in host CWD, autoconfigure ATMOS_BASE_PATH #756
Conversation
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 don't have any problems with the actual technicalities of this PR — to be honest there is a lot of shell parameter expansion syntax that is best understood by the author iteratively writing _workdir.sh
and atmos.sh
, so it's hard to nitpick anything there unless I spend a ton of time on it.
But I did leave a couple comments on what's printed out to the shell and also the grouping of if conditionals.
# use it as the $ATMOS_BASE_PATH | ||
if [[ -d "${GEODESIC_WORKDIR}/stacks" ]] && [[ -d "${GEODESIC_WORKDIR}/components" ]]; then | ||
export ATMOS_BASE_PATH="${GEODESIC_WORKDIR}" | ||
green "# Setting ATMOS_BASE_PATH to \"$ATMOS_BASE_PATH\" based on children of workdir" |
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.
"Children of workdir" is a little vague outside of the context, especially to a Geodesic user who isn't reading the source of this script.
Here's a quick suggestion, but maybe word it differently.
green "# Setting ATMOS_BASE_PATH to \"$ATMOS_BASE_PATH\" based on children of workdir" | |
green "# Atmos base path automatically determined to be current workdir; setting ATMOS_BASE_PATH to \"$ATMOS_BASE_PATH\"" |
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.
What is printed out is in a form and style consistent with the other startup output from Geodesic:
# Mounting /Users/user into container
# Starting new geodesic session from cloudposse/geodesic:debian
# Exposing port 32380
# Setting ATMOS_BASE_PATH to "/localhost/source/geodesic" based on children of workdir
The fact that "children of workdir" is a bit cryptic is OK with me. The purpose of the message is not to explain to the user how the setting was derived, as that just takes too much verbiage for a startup message. The point is to distinguish which criteria were used in setting ATMOS_BASE_PATH
so if there is a problem, we can more easily diagnose the cause.
if [[ -n $ATMOS_BASE_PATH ]]; then | ||
if [[ $SHLVL == 1 ]]; then |
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.
Can't this be part of the same if
conditional? i.e. [[ -n $ATMOS_BASE_PATH ]] && [[ $SHLVL == 1 ]]
?
Also I think there should be an inline comment explaining why we are specifically testing for the number of shell levels.
EDIT: I see that they do need to be nested. But my comment on SHLVL
still applies.
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.
There is a classic example of a bad comment:
x = x + 1 // Add 1 to x
I didn't put a comment in about SHLVL
because it seems to me it would be too much like that kind of bad comment.
# Only output status in top level shell
if [[ $SHLVL == 1 ]]; then
The more useful comment would be about the bigger architectural design choice, but that would be long and does not belong in this bit of code. Since you asked, I will explain here in this comment.
The startup scripts (everything in profile.d
) are responsible for a lot of the behavior of the interactive shell. Every time we spawn an interactive subshell, such as with assume-role
, we run all these scripts again in the subshell to set it up. However, the startup messages are intended to expose configuration details of the Docker container or, if you will, the Geodesic session, more generally, and we do not want to repeat them when starting up a subshell with assume-role
because (1) at that point they are not news and (2) its unnecessarily distracting. If you look around, you will find test for $SHLVL == 1
all over the scripts in profile.d
.
what
atmos
configuration, setATMOS_BASE_PATH
accordinglykubectl
1.16 -> 1.21why
kubectl
should have been upgraded long agoreferences
base_path
setting toatmos
config atmos#100notes
Customizing command line prompt appearance
You could previously choose between 3 sets of glyphs to be used in the Geodesic command line prompt by setting the environment variable
PROMPT_STYLE
to "plain" to use plan ASCII characters, "unicode" to use Unicode characters limited to the standard (not supplemental) planes of Unicode, or leave it unset (or set to anything else) to get the default set of glyphs, which notably includes '⨠' Z NOTATION SCHEMA PIPING from the "Supplemental Mathematical Operators" Unicode block. Being from the supplemental plane, it is not as widely available: for instance, it is not included in the default fonts on Ubuntu.Our current recommendation for resolving this issue is to install a font that contains the missing character on your host. For example, for Ubuntu:
However, taking this as a general issue rather than just specific to Ubuntu, we have included more options.
New PROMPT_STYLE: You can now set
PROMPT_STYLE=fancy
to get the default set of glyphs except for the one missing from Ubuntu, which is replaced with the glyph from the "unicode" style. This should cover most cases where the default style is broken.GEODESIC_PROMPT_GLYPHS: We previously provided a specific workaround for this by allowing you to set
GEODISIC_PROMPT_GLYPHS
[sic] to any string of characters you wanted to end the prompt with. In this release we fixed the spelling of that environment variable, so it is nowGEODESIC_PROMPT_GLYPHS
, but hopefully you will no longer need it, and you will instead choose either to install a font or switch to the "fancy" style.Full glyph customization: You can now set each of the 4 glyphs used by the command line prompt, plus the host file system designator, individually:
ASSUME_ROLE_ACTIVE_MARK
is the glyph to show when you have AWS credentials active. Defaults to a green, bold, '√' SQUARE ROOT (looks like a check mark):$'\u221a'
ASSUME_ROLE_INACTIVE_MARK
is the glyph to show when you do not have AWS credentials active. Defaults to a red, bold, '✗' BALLOT X:$'\u2717'
BLACK_RIGHTWARDS_ARROWHEAD
is the glyph at the end of the prompt. The troublesome default is '⨠' Z NOTATION SCHEMA PIPING:$'\u2a20'
BANNER_MARK
is the glyph at the start of the first line of a 2-line prompt that introduces theBANNER
text. Defaults to '⧉', TWO JOINED SQUARES:$'\u29c9'
PROMPT_HOST_MARK
is added to the command line when the current working directory is on the host computer (via a bind mount) and not in the container. Defaults to '(HOST)' with "HOST" in red bold. Disable this feature by settingexport PROMPT_HOST_MARK=""
.You can now set any of these in your standard customizations and your setting will be used.
Cautions:
$'\x01'
and ending it with$'\x02'
, but there a couple of other, similar options. If you fail to do this, or do it incorrectly, your cursor will be incorrectly positioned when you edit a command line from your history and what you see will not be what is executed.