Skip to content

Conversation

@vdice
Copy link
Member

@vdice vdice commented Jul 14, 2016

if this pipeline is triggered by a PR in controller-sdk-go

Ref deis/controller-sdk-go#39
Ref deis/controller-sdk-go#41

@vdice vdice added this to the v2.2 milestone Jul 14, 2016
@vdice vdice self-assigned this Jul 14, 2016
@deis-bot
Copy link

@Joshua-Anderson is a potential reviewer of this pull request based on my analysis of git blame information. Thanks @vdice!

@vdice vdice force-pushed the receive-sdk-params branch 17 times, most recently from f02ac4c to 2277c09 Compare July 14, 2016 16:41
Jenkinsfile Outdated

if (env.SDK_GO_REPO && env.SDK_SHA) {
echo "Updating local glide.yaml with controller-sdk-go repo '${env.SDK_GO_REPO}' and version '${env.SDK_SHA}'"
sh "perl -i -0pe 's/github\\.com\\/deis\\/controller-sdk-go\\n\\s+version:\\s+[a-f0-9]+/${env.SDK_GO_REPO}\\n version: ${env.SDK_SHA}/' glide.yaml"
Copy link
Member Author

@vdice vdice Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had prev been trying the groovy route of:

def pattern = ~/github.com\/deis\/controller-sdk-go\n\s+version:\s+[a-f0-9]+/
def g = new File('glide.yaml')
text = g.text
g.withWriter { w ->
  w << text.replaceAll(pattern, "${env.SDK_GO_REPO}\n  version: ${env.SDK_SHA}")
}

Which is acceptable groovy and runs great in the console, but the Jenkins Secure Script Plugin warns not to approve use of all/part of it:

signature : new java.io.File java.lang.String Approving this signature may introduce a security vulnerability! You are advised to deny it.

(Can be seen here as well)

@vdice vdice force-pushed the receive-sdk-params branch 3 times, most recently from 31254f6 to 219eb9a Compare July 14, 2016 18:20
@vdice
Copy link
Member Author

vdice commented Jul 14, 2016

💥 first build with successful use of PR sdk go repo & sha: https://ci.deis.io/job/Deis/job/workflow-cli/job/PR-128/39/console

@bacongobbler
Copy link
Member

This'll be awesome for testing PRs like #118!

@vdice
Copy link
Member Author

vdice commented Jul 15, 2016

This PR should be merged before deis/controller-sdk-go#41

def glideYaml = readFile('glide.yaml')
echo "Updated glide.yaml:\n${glideYaml}"

sh 'make glideup'
Copy link
Member

@arschles arschles Jul 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one note here - this build target will run glide up (obviously) which may update other dependencies, resulting in a build environment different from that which is specified in the PR's glide.lock...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arschles shall we create a ticket to find an alternative to glide up? Or, if one is available immediately, should we use it here? (only update a particular package would be ideal, right?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, particular package would be ideal here. I'm not really sure of a good alternative off the top of my head 😦

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why wouldnt we just do a glide install?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are updating the controller-sdk-go package's version number, which then glide.lock is out of date so glide install won't work in this case.

@arschles
Copy link
Member

@vdice left a note at #128 (comment), which may be an issue going forward, but I don't believe is a blocker for this particular PR. @bacongobbler since you also commented on this PR, do you have any thoughts on the issue detailed there?

Jenkinsfile Outdated
[$class: 'BooleanParameterDefinition',
defaultValue: true,
description: 'option to run e2e tests (default: true)',
name: 'RUN_E2E']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

echoing my comment at echoing my comment at https://github.com/deis/controller-sdk-go/pull/41/files#r71024488 - where do this and the above env vars come from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arschles they can be passed down from any upstream job that chooses to do so (for our intended case, they are passed down by the upstream controller-sdk-go job for PRs in the code here). If, however, no upstream job chooses to pass values for those, you can see the defaults values are empty strings (and thus the glide.yaml modification logic will not be executed).

That being said, I need to update this PR to remove the RUN_E2E -- I intend to PR this functionality separately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now updated (RUN_E2E property removed)

@vdice vdice force-pushed the receive-sdk-params branch from 219eb9a to 0804968 Compare July 15, 2016 19:17
if this pipeline is triggered by a PR in controller-sdk-go

Ref deis/controller-sdk-go#39
@bacongobbler
Copy link
Member

bacongobbler commented Jul 18, 2016

not a blocker, but yeah a glide up for a specific package (e.g. glide up github.com/deis/controller-sdk-go) would be nice to file upstream. LGTM

@vdice vdice merged commit c2018d4 into deis:master Jul 18, 2016
@vdice vdice deleted the receive-sdk-params branch July 18, 2016 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants