-
Notifications
You must be signed in to change notification settings - Fork 196
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
Update env:deploy ACs #1238
Update env:deploy ACs #1238
Conversation
- Update site and env flags to a single argument in the form <site>.<env> - Update required deploy note from flag to argument
@ari-gold, thanks for your PR! By analyzing the blame information on this pull request, we identified @TeslaDethray to be a potential reviewer |
@@ -9,7 +9,7 @@ Feature: Site Deployment | |||
|
|||
@vcr site_deploy | |||
Scenario: Deploy dev to test | |||
When I run "terminus env:deploy test --site=[[test_site_name]] --sync-content --note='Deploy test'" | |||
When I run "terminus env:deploy [[test_site_name]].test 'Deploy log note' --sync-content" |
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.
@ari-gold: Is the deploy log note optional? If it is I think I would keep the --note|-n
flag so that the position of optional arguments is not significant... it could be specified before or after --sync-content
which is also optional. If --note
is not optional then I'm +1 on this arg layout.
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 I am not -1 on the way it is now :-)
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.
- Rather don't change the notes since that requires recording new fixtures.
- I agree with Adam, if it's optional it should be an option, and existing functionality has the note default to "Deploy from Terminus" so it's currently optional.
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.
Agreeing with @ajbarry and @marktheunissen, the note should be an option.
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.
When I tested existing functionality the note was required:
$ terminus site deploy --env=test --site=aaa-ari
Custom note for the deploy log:
Custom note for the deploy log:
Custom note for the deploy log:
Custom note for the deploy log:
Custom note for the deploy log:
Custom note for the deploy log:
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.
@ari-gold Terminus requires it now, but the API does not require it. Why be beholden to the old pattern?
https://github.com/pantheon-systems/titan-mt/blob/master/yggdrasil/yggdrasil_workflows.py#L4448
site.env