-
Notifications
You must be signed in to change notification settings - Fork 313
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
Initial CLI Refactor #241
Initial CLI Refactor #241
Conversation
3c7c5e0
to
ac264e8
Compare
ac264e8
to
282a6f7
Compare
Commands for listing attributes of stacks. | ||
|
||
""" | ||
pass |
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.
Should the reason for this be documented? I'm uncertain.
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.
Leaving as cli group behaviours are documented with Click documentation
|
||
""" | ||
stack = get_stack(ctx, path) | ||
stack.execute_change_set(change_set_name) |
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.
Is there another place where this function could be placed?
|
||
|
||
@contextlib.contextmanager | ||
def change_set(stack, name): |
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 seems to me like there is a better place for this function than 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.
Can remove all together
return logger | ||
|
||
|
||
def simplify_change_set_description(response): |
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.
Is there a good reason for defining this function here and describe.py?
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.
Yup duplicated function - removed from describe.py and imported from here
@@ -0,0 +1,151 @@ | |||
import click |
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.
Name of this file is slightly misleading, since it contains all of the "read only" operations, instead of just the describe commands.
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 - so realised I can't split out "list" commands in to a list.py and import list, because it clashes with built-in python keyword "list". Not sure what to rename too?
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.
Creating extra list.py file
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.
Expanding on this, we discussed offline that we can just do
from list import list_group
for the purposes of using that module within Sceptre. It is more verbose, but it makes the structure of the commands more intuitive and, thus, more maintainable.
This may cause confusion if someone wanted to use Sceptre as a Python module and import the Sceptre list.py; however, we don't officially support using cli commands as modules and users may sidestep this issue by adding the import statement:
import list as list_commands
instead.
Requires #259 before merging. |
This PR refactors the CLI into the following:
The CLI has been reorganized in to a folder
cli
splitting up commands into separate files. Thecontinue-update-rollback
command has been removed due to improvements in CloudFormation and maintain cli consistency.[Resolve #issue-number]
(if a related issue exists).make test
) are passing.make lint
) offenses.and description in grammatically correct, complete sentences.