Snapshot for standalone planner #378

pranavgore09 · 2018-02-07T05:20:52Z

Added a new groovy script that will deploy snapshot and report the route URL on the PR.
Similar to deployOpenShiftSnapshot

This is being consumed in fabric8-ui/fabric8-planner#2426

jaseemabid · 2018-02-08T08:55:17Z

vars/deployPlannerSnapshot.groovy

+    body()
+
+    def yaml
+    def openShiftProject = config.openShiftProject


Why not just use config.openShiftProject instead of using an additional local variable?

Since config is set to empty dictionary in line 7, isnt config.openShiftProject going to be empty here always?

I have used several times later in the code, instead of reading every time from the map I thought local var is good. Not sure though.

.delegate does kind of magic, it will export values passed to function and keep filling up the empty config with provided values if I get it right.

jaseemabid · 2018-02-08T08:56:50Z

vars/deployPlannerSnapshot.groovy

+    def originalImageName = config.originalImageName
+    def newImageName = config.newImageName
+    def deploymentName = config.githubRepo
+    def providerLabel = config.providerLabel ?: 'fabric8'


All local variables except this would be null. Maybe you forgot a config param in the function signature?

confuses me as well, and follows what is done in https://github.com/fabric8io/fabric8-pipeline-library/blob/master/vars/deployOpenShiftSnapshot.groovy#L11 Is there some magic involved?

as mentioned above in comment - https://github.com/fabric8io/fabric8-pipeline-library/pull/378/files#diff-e29e1891af9b97dd7f7137d912b468acR9 this line is responsible

@jaseemabid wonder why you thought all local variables would be null? wouldn't body() initialise these?

@pranavgore09 @jaseemabid hopefully this may make sense to you ...

class Body { def closure = { println delegate println "closure: foo: ${foo}" foo = 'barrrr' bar = 'baaazzz' } } def config = [ foo: "bar"] def obj = new Body() def body = obj.closure body.delegate = config println "main: foo: ${config.foo}" println "main: bar: ${config.bar}" body() println "main: foo: ${config.foo}" println "main: bar: ${config.bar}"

Output:

main: foo: bar main: bar: null class delegate closure: foo: bar main: foo: barrrr main: bar: baaazzz

just in case you are wondering where I got this idea from :) - https://stackoverflow.com/a/23450944

jaseemabid · 2018-02-08T08:57:31Z

vars/deployPlannerSnapshot.groovy

+    def flow = new io.fabric8.Fabric8Commands()
+    def utils = new io.fabric8.Utils()
+
+    openShiftProject = openShiftProject + '-' + utils.getRepoName()


Define the variable here, avoid that mutation

yes done, now only one declaration in a869abe

jaseemabid · 2018-02-08T08:58:34Z

vars/deployPlannerSnapshot.groovy

+    }
+    // cant use writeFile as we have long filename errors
+    sh "echo '${yaml}' > snapshot.yml"
+


There is probably a groovy function called write, so that we dont have to spawn a subshell here. There must be something similar to write('snapshot.yml', yaml)

kept is as it is, but added the error that I faced in a comment above it a869abe#diff-e29e1891af9b97dd7f7137d912b468acR39

jaseemabid · 2018-02-08T09:03:48Z

vars/deployPlannerSnapshot.groovy

+    def utils = new io.fabric8.Utils()
+
+    openShiftProject = openShiftProject + '-' + utils.getRepoName()
+    container('clients') {


Spawning the clients container just to get oc binary seems a bit too much. Should probably add the 'oc' binary to the jenkins builder image and scrap this. Need not be part of this PR obviously, but something that needs a look. WDTY @rupalibehera @chmouel @sthaha

I am not sure what you meant by jenkins builder image; not an area I know. is it the jenkins master image?

@sthaha Yes. I meant master. Looks like we bring in yet another repository and image and container just for the oc binary from https://github.com/fabric8io-images/builder-clients, which is what I was hoping to get rid of.

jaseemabid · 2018-02-08T09:05:49Z

vars/deployPlannerSnapshot.groovy

+                return true
+            } catch (err) {
+                echo "waiting for ${deploymentName} to be ready..."
+                return false


Is this waitUntill a tight loop? Do you think there should be a sleep if it fails? Is that inbuilt? If we add a sleep before the return false, we can remove the sleep 10 above?

sthaha

Nice patch! Looks good to me, some minor comments thou.

sthaha · 2018-02-08T09:03:55Z

vars/deployPlannerSnapshot.groovy

+    body()
+
+    def yaml
+    def openShiftProject = config.openShiftProject


how about def openshiftProject = config.openshiftProject + '-' + utils.getRepoName()
so that this variable isn't overwritten in line 24?

yes! done in a869abe

sthaha · 2018-02-08T09:05:55Z

vars/deployPlannerSnapshot.groovy

+    def flow = new io.fabric8.Fabric8Commands()
+    def utils = new io.fabric8.Utils()
+
+    openShiftProject = openShiftProject + '-' + utils.getRepoName()


IMHO, keeping variables as immutable as possible makes it easy to reason with the code. Here openshiftProject isn't acting like an accumulator (sum, product etc...) so we can keep it immutable like mentioned in the comment above

👍 fixed a869abe

sthaha · 2018-02-08T09:08:39Z

vars/deployPlannerSnapshot.groovy

+            error 'Change author is not a collaborator on the project, aborting build until we support the [test] comment'
+        }
+
+        yaml = flow.getUrlAsString(openShiftTemplate)


same pattern here, made me think that yaml is getting overwritten in the line below. how about:

def url = flow.getUrlAsString(openshiftTemplate) def originalImage = "- image: ${...}" def newImage = "- image: ${...}" yaml = Pattern.compiler(originalImage).matcher(url).replaceFirst(newImage)

thanks, fixed.

sthaha · 2018-02-08T09:09:29Z

vars/deployPlannerSnapshot.groovy

+    sh "echo '${yaml}' > snapshot.yml"
+
+    container('clients') {
+        try {


we could convert this into a reusable method switchProject(project)

jaseemabid · 2018-02-08T09:11:15Z

@pranavgore09 Several comments inline. AFAIK, it fundamentally boils down to a

wget openShiftTemplate > snapshot.yml 
oc apply -f snapshot.yml

I guess a straightforward shell script would look a lot saner than so many levels of indirection caused by this library. The patch looks fine & it seems to fit in with the rest of the code. LGTM except the inline comments.

Added new groovy script for building and deploying plannerUI standalone snapshot in CI.

pranavgore09 · 2018-02-13T09:51:56Z

@jaseemabid @sthaha I have removed container(clients) where it was not required and variables are not modified after assignment.
please review once! thanks.

jaseemabid · 2018-02-14T05:23:34Z

vars/deployPlannerSnapshot.groovy

+    def originalImage = "- image: ${originalImageName}:(.*)"
+    def newImage = "- image: ${newImageName}"
+    def compiledYaml = Pattern.compile(originalImage).matcher(yaml).replaceFirst(newImage)
+


@pranavgore09 Its slightly odd that we use string matching and regular expressions to get data out of a structured data interchange format. I would have expected to see something like this.

def yaml = Yaml.parse(flow.getUrlAsString(openShiftTemplateURL)) yaml.services[1].image = config.newImage // Whatever the real path is def snapshot = yaml.toString() // Remove the check below, because that wont fail now.

I'm OK with this code, this might be written this way because of other nuances like groovy sandbox or something which we don't know.

Point being, this feels like a very poor groovy implementation of

wget openShiftTemplate > snapshot.yml sed -i -e 's/oldImage/newImage/g' snapshot.yml

and that feels really odd.

It indeed has a method to parse yaml but @jaseemabid mentioned I feel this is consistent with the existing implementation so lets go ahead with this and fix both later as a separate issue.

sthaha

Looks good to me

sthaha · 2018-02-14T05:34:02Z

vars/deployPlannerSnapshot.groovy

+    def originalImageName = config.originalImageName
+    def newImageName = config.newImageName
+    def deploymentName = config.githubRepo
+    def providerLabel = config.providerLabel ?: 'fabric8'


@jaseemabid wonder why you thought all local variables would be null? wouldn't body() initialise these?

sthaha · 2018-02-14T05:37:10Z

vars/deployPlannerSnapshot.groovy

+    def originalImage = "- image: ${originalImageName}:(.*)"
+    def newImage = "- image: ${newImageName}"
+    def compiledYaml = Pattern.compile(originalImage).matcher(yaml).replaceFirst(newImage)
+


It indeed has a method to parse yaml but @jaseemabid mentioned I feel this is consistent with the existing implementation so lets go ahead with this and fix both later as a separate issue.

jaseemabid · 2018-02-14T05:52:08Z

LGTM. Merge 👍

bartoszmajsak · 2018-02-15T15:09:09Z

Just a general thought I wanted to share - this kind of functions should not belong to the "library" as they are very specific to particular projects. That makes them unshareable essentially. So either we could decompose it to more "generic purpose" functions and reuse from within Planner Jenkinsfile or maybe create it's own "planner-pipeline-library" with those bits instead? (I believe one can have more than one @Library reference in the Jenkinsfile itsefl - see here for more details https://jenkins.io/doc/book/pipeline/shared-libraries/.

/cc @pradeepto @joshuawilson

joshuawilson · 2018-02-15T15:26:59Z

I have questioned this approach of one-off functions since we started using it. I don't know how we can move away from it quickly. So we might just accept this but work toward a better solution.

bartoszmajsak · 2018-02-15T15:30:28Z

@joshuawilson timebox it for 1 day for someone in your team and try? I think it's easily done based on the guide I shared above.

pranavgore09 · 2018-02-15T15:53:57Z

thanks, @bartoszmajsak @joshuawilson for taking a look, but when I started working on this, my intention was to deploy snapshot for the standalone planner, over a period of time it changed to a greater extent and as of now that change does not have any static things in it, any specific setup inside. Except the name of the file itself.

essentially it deploys application given in openshiftTemplate. here

The only line is regex matcher that seems to be static, but IMO that is fine.
WDYT @jaseemabid @sthaha @bartoszmajsak @joshuawilson about changing its name to something more meaningful for a library

bartoszmajsak · 2018-02-15T16:25:23Z

Thanks for explanation @pranavgore09, makes sense :) Nice work! My comment was a more a general statement.

Maybe in this given case we only need to rename the function then. But we do have a lot of other fabric8..Deploy... functions which are project specific - this needs to be addressed as part of openshiftio/openshift.io#2243.

jaseemabid · 2018-02-16T08:30:41Z

@bartoszmajsak @joshuawilson There is a whole lot of project specific code in the library and I think we all agree that it doesn't make much sense.

We will need to take a look at it as a whole and trim down a lot. For example, #372

pranavgore09 mentioned this pull request Feb 7, 2018

Snapshot for standalone planner UI fabric8-ui/fabric8-planner#2426

Closed

pranavgore09 force-pushed the standalone-planner branch 2 times, most recently from ee99ef0 to 0b23291 Compare February 8, 2018 08:57

jaseemabid suggested changes Feb 8, 2018

View reviewed changes

sthaha reviewed Feb 8, 2018

View reviewed changes

pranavgore09 force-pushed the standalone-planner branch from cc2a0fb to a869abe Compare February 9, 2018 13:31

Deploy plannerUI snapshot

0490a4d

Added new groovy script for building and deploying plannerUI standalone snapshot in CI.

pranavgore09 force-pushed the standalone-planner branch from a246360 to 0490a4d Compare February 13, 2018 09:50

jaseemabid approved these changes Feb 14, 2018

View reviewed changes

sthaha approved these changes Feb 14, 2018

View reviewed changes

bartoszmajsak mentioned this pull request Feb 15, 2018

Audit the existing fabric8 pipeline library and create a new OSIO pipeline library if need be. openshiftio/openshift.io#2243

Closed

3 tasks

Snapshot for standalone planner #378

Are you sure you want to change the base?

Snapshot for standalone planner #378

Conversation

pranavgore09 commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavgore09 Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavgore09 Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavgore09 Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavgore09 Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaseemabid commented Feb 8, 2018

pranavgore09 commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaseemabid commented Feb 14, 2018

bartoszmajsak commented Feb 15, 2018

joshuawilson commented Feb 15, 2018

bartoszmajsak commented Feb 15, 2018

pranavgore09 commented Feb 15, 2018 • edited Loading

bartoszmajsak commented Feb 15, 2018

jaseemabid commented Feb 16, 2018

sthaha Feb 14, 2018 •

edited

Loading

pranavgore09 Feb 9, 2018 •

edited

Loading

pranavgore09 Feb 9, 2018 •

edited

Loading

pranavgore09 Feb 9, 2018 •

edited

Loading

pranavgore09 Feb 9, 2018 •

edited

Loading

pranavgore09 commented Feb 15, 2018 •

edited

Loading