-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create CGAP production env auto-deploy #303
Conversation
To be deployed on the same schedule as fourfront staging.
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.
One change is necessary for this to work, other than that we can discuss how to test this prior to merging.
def deploy_ff_staging(connection, **kwargs): | ||
""" Deploys Fourfront master to whoever staging is. | ||
Runs as part of the 'deployment_checks' schedule on data ONLY. | ||
def deploy_env(connection, env_to_deploy, name, check, **kwargs): |
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.
Having the @check_function
decorator here will break things as it is not a check function itself but a helper function to run the below two checks. You cannot "nest" checks this way, see the decorator code.
""" Deploys Fourfront master to whoever staging is. | ||
Runs as part of the 'deployment_checks' schedule on data ONLY. | ||
def deploy_env(connection, env_to_deploy, name, check, **kwargs): | ||
""" For a given beanstalk environment, deploys, or returns an error. |
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 would at least say what name
is in the docstring as it not clear that by name
you mean application_name
.
- clearer docstring (ended up clearing up name as well) - correct use of check_function decorator
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.
No blockers here.
if helper_check.status == 'PASS': | ||
this_check.status = 'PASS' | ||
this_check.summary = 'Successfully deployed Fourfront master to %s' % env_to_deploy | ||
this_check.summary = 'Successfully deployed {0} master to {1}'.format(application_name, env_to_deploy) |
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.
Minor: I have a very small preference, once you have multiple args, for named arguments to format, which is easier to keep track of visually than numbers, as in (untested):
this_check.summary = ('Successfully deployed {what} master to {where}'
.format(what=application_name, where=env_to_deploy))
env_to_deploy=compute_cgap_prd_env(), | ||
application_name="CGAP Portal", | ||
check='deploy_cgap_production', | ||
**kwargs) |
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.
newline needed
To be deployed on the same schedule as fourfront staging.