-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Adding sample Jenkins Pipeline #16880
Adding sample Jenkins Pipeline #16880
Conversation
@bparees @gabemontero ptal |
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 not a "detailed walkthrough/example of building/deploying/promoting an application via a pipeline." (from the trello card).
it's also using the legacy plugin logic ("openshiftBuild", "openshiftVerifyBuild", etc) when it should be using only the openshift client plugin.
jenkinsfile: |- | ||
try { | ||
timeout(time: 20, unit: 'MINUTES') { | ||
// run this pipeline on a node.js slave pod |
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.
align this w/ the next line.
// setup to check if the objects from the template have already been created | ||
def templateGeneratedSelector = openshift.selector('all,secret', [ template: 'nodejs-mongodb-example' ]) | ||
def objectsGeneratedFromTemplate = templateGeneratedSelector.exists() | ||
def objectModels = openshift.process( template ) |
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 reason to process the template if the objects already exist.
some additional information on why i say this... all this pipeline is doing is deploying a template if the template was not previously deployed. That's not a useful workflow for someone trying to iterate on developing an application because it's not going to update things if their template has changed. it's also not demonstrating promotion. what would make more sense:
and it should be done using the openshift client plugin semantics, not the openshift pipeline plugin ones. |
@bparees @gabemontero ptal |
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.
by reusing an existing template you've created a somewhat unnatural pipeline definition. i think it would be better to have your own template, or to invoke oc new-app to construct all the resources dynamically.
} | ||
stage('create') { | ||
// create a new template from the templatePath | ||
def template = openshift.create(templatePath).object() |
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.
you shouldn't need to create the template to process it.
for (o in objectModels) { | ||
// update the output image to be tagged with 'dev' | ||
if (o.kind == "BuildConfig") { | ||
o.spec.output.to.name = "${templateName}:dev" |
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.
why doesn't the template just define it this way?
for (t in o.spec.triggers) { | ||
// set the deployment config to deploy the image tagged with 'dev' | ||
if (t.type == "ImageChange") { | ||
t.imageChangeParams.from.name = "${templateName}:dev" |
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.
why doesn't the template just define it this way?
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've got nothing new to add other than two of the points I raised in the associated doc PR ... I'll reiterate here for convenience:
- long term we should monitor if adding a Jenkinsfile along these lines in our -ex repos makes sense
- if not now, at some point soon we'll need a declarative pipeline syntax example
Updated example to use declarative pipeline syntax |
/retest |
1 similar comment
/retest |
/lgtm |
@bparees ptal, all tests are passing |
kind: "BuildConfig" | ||
apiVersion: "v1" | ||
metadata: | ||
name: "sample-nodejs-pipeline" |
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.
align this name and the filename.
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.
updated, thought i had already fixed that, must have been in the docs
// if everything else succeeded, tag the ${templateName}:latest image as ${templateName}:stage | ||
// a pipeline build config for the staging environment can watch for the ${templateName}:stage | ||
// image to change and then deploy it to the staging environment | ||
openshift.tag("${templateName}:latest", "${templateName}:stage") |
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.
you need to tag this into a different imagestream because your cleanup logic deletes the ${templateName} imagestream every time. And having a pipeline that deletes the "last good" tag as its first step is not logical.
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.
updated
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.
(also updated the example in the docs)
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, coreydaley, gabemontero The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Adding sample Jenkins Pipeline using declarative syntax Adding a sample Jenkins Pipeline in support of documentation updates. Trello: https://trello.com/c/rBojNLGj/1121-5-better-devguide-pipeline-docs-techdebt
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
1 similar comment
/retest |
/cherrypick stage |
@Kargakis: @Kargakis once the present PR merges, I will cherry-pick it on top of stage in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Automatic merge from submit-queue. |
Adding a sample Jenkins Pipeline in support of documentation updates.
Supports: openshift/openshift-docs#5720
Trello: https://trello.com/c/rBojNLGj/1121-5-better-devguide-pipeline-docs-techdebt