Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Start in host CWD, autoconfigure ATMOS_BASE_PATH #756
Changes from all commits
1149442
8b274dd
48eb9cc
9fd997c
94bfa28
dbc69f8
3991ebe
6a6649d
f275b01
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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:
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.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 withassume-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 withassume-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 inprofile.d
.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.
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:
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.