-
Notifications
You must be signed in to change notification settings - Fork 55
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
Jenkinsfile.release: make AWS AMI replication optional #132
Conversation
9c9af5a
to
ef3e822
Compare
ok you can specify a boolean var for false like this:
|
ef3e822
to
fd217e6
Compare
ok I switched to using strings for the parameters. Now ready for review. |
fd217e6
to
be5593d
Compare
Jenkinsfile.kola.aws
Outdated
@@ -55,6 +55,7 @@ podTemplate(cloud: 'openshift', label: 'coreos-assembler', yaml: pod, defaultCon | |||
|
|||
stage('AWS Kola Run') { | |||
utils.shwrap(""" | |||
export AWS_CONFIG_FILE=\${AWS_FCOS_KOLA_BOT_CONFIG} |
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.
Previously the AWS config was pre-validated by nature of the build having been uploaded with the credentials being used to test. With this change to using a different config we should validate that it exists before attempting this step.
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.
This commit was part of #130 - I'll address it in a followup PR.
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.
LGTM, tiny optional comments.
Jenkinsfile
Outdated
choices: (['false', 'true']), | ||
defaultValue: 'false', | ||
description: 'Force AWS AMI replication for non-production', | ||
required: false) |
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/optional: required: false
is the default
// Since we are only running this stage for non-production (i.e. mechanical | ||
// and development) builds we'll default to not doing AWS AMI replication. | ||
// That can be overridden by the user setting the AWS_REPLICATION parameter | ||
// to true, overriding the default (false). |
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/optional: seems like this would belong better near the default: 'false'
line?
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.
going to leave it where it is for now. I know it's just comments, but I don't want to re-test.
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.
err rather I don't have the ability to re-test right now.
be5593d
to
a9ed30d
Compare
a9ed30d
to
826f95e
Compare
And default to true, but make the pipeline default to not replicate for non-production streams. We use strings and not boolean parameters because passing booleans via `oc start-build -e` is non-trivial.
826f95e
to
fd798ec
Compare
And default to true, but make the pipeline default to not replicate
for non-production streams.