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
sam pipeline bootstrap #2811
sam pipeline bootstrap #2811
Changes from 9 commits
83fc54a
9f0147a
3946009
d3db05b
201de2e
1d2cc06
588d4ab
46402f0
9747413
8a7404e
6e3f21d
58e48a2
206b7e3
23148ff
37a4f5f
8ea4153
99c91c8
b4c248f
3acea8c
99bef0b
eacfe9c
5fdd32a
83712cd
2c3ad8b
d184e16
6d9fb34
0f1152c
1721c35
15b64d9
b1e31c0
050827a
0444e0c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
same as above, AccessKeyId and SecretAccessKey
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.
"pipeline-user" -> "pipeline user"
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 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 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 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.
Above we wrote:
I think we need to find a consistent way to describe
PackageType=Image
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 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 not apparent what format customers should use here. Maybe provide some examples
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 are the possible exception types here?
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.
The intention is to capture all kind of exceptions(including unexpected ones); At this point the resources are already bootstrapped, we don't prompt/confirm with the users that we are going to save these configs, i.e. they are unaware of that. So, the bootstrap command has already succeeded and the users should face any failure messages.
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 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.
why do you want to add docstring to a private method?
The method that do the actual logic, i.e.
get_cmd_names
is already documentedaws-sam-cli/samcli/cli/context.py
Lines 190 to 204 in d479678
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.
Because it is not intuitively clear what this function will output. If I just look at it, it involves
click
and another function. Without the click context, docstring can help to understand what it does.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.
Besides,
get_cmd_names()
is a confusing function itself.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.
Ummmm, are you saying the function name
_get_command_name
doesn't clearly indicates that this function returns the name of the command?this function just combine two lines of code together instead of duplicating them every time.
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.
yeah.. the confusing part is, which of the following is the output?
["sam", "pipeline", "bootstrap"]
["sam", "pipeline"]
["pipeline", "bootstrap"]
If I cannot get the answer by looking at the function code, I think it deserves a docstring (comment)
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.
then this should be fixed in the public method
get_cmd_names()
itself, right?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.
Instead of
cast
s would be better to update the function type hints.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 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 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.
besides, we should use
if ... template includes
instead ofwill include
here. See https://writingcenter.unc.edu/tips-and-tools/conditionals-verb-tense-in-if-clauses/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 used the future tense to let the users think about the future; Instead of thinking "no my templates doesn't contain this" they will think "will my template contain this".
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.
but it is not how "if" is used in English as far as I know. You can check the article I posted above, they cover all scenarios and none of them use future tense. (specifically about the "likely")
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.
more reading: https://www.grammarly.com/blog/will-would-in-if-clause/
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.