-
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
pipeline: upload builds to S3 #59
Conversation
Jenkinsfile
Outdated
@@ -120,6 +138,12 @@ podTemplate(cloud: 'openshift', label: 'coreos-assembler', yaml: pod, defaultCon | |||
// https://stackoverflow.com/questions/1636889 | |||
if (!devel) { | |||
utils.rsync_out("builds", "builds") | |||
if (utils.path_exists("/.aws")) { |
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.
any preference on just using ENV vars here instead of a config file there?
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.
Heh, I explicitly chose to move away from env vars actually to raise the barrier a bit more on how easily credentials could leak e.g. into logs.
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.
Side note, for AWS it's best to use https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_temp_use-resources.html if you can.
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.
Heh, I explicitly chose to move away from env vars actually to raise the barrier a bit more on how easily credentials could leak e.g. into logs.
yeah. i've just always done it the other way so I have a slight preference. We can stick with 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.
Side note, for AWS it's best to use https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_temp_use-resources.html if you can.
Hmm neat, hadn't seen this before. I don't want to address this right now in this patch, but I don't want to lose track of it either, so filed #63.
Explicitly check for the exact `JENKINS_URL` and `JOB_NAME` we expect for the prod job. Anything else is a "devel" job. Prep for supporting devel pipelines.
Prep for adding more git-related parameters.
OK, this is updated and tested now! You can use #62 to test this:
Wait for build to finish. Then e.g.:
One obvious thing here is that we're re-using the same workdir as prod, though that's not an urgent issue right now since (1) Jenkins won't run them concurrently and (2) we always rsync in the prod data from the artifact server. Still, once we move away from the artifact server (which we should be able to do as soon as this code runs once), I'll clean that up. |
This is fixed in #62 now. |
gave this a quick review - had one comment. will try out with devel pipelines next. |
This is a first step towards switching to S3. We still rsync to the artifact server, but we also upload new builds to S3. Once we confirm this works nicely, we can switch over to using `buildprep` and completely wean off the artifact server.
Assuming this PR is updated with all the content from #62 this LGTM |
This is a first step towards switching to S3. We still rsync to the
artifact server, but we also upload new builds to S3. Once we confirm
this works nicely, we can switch over to using
buildprep
andcompletely wean off the artifact server.
Requires: coreos/coreos-assembler#527
I tested this from my local cluster and it works, though I'm also working on #57 to make it easier to test changes like these.