major refactor to how parameters work

[itewk]

Purpose

Make it easier to consume this library while also maintaining scalability and maintainability of the library itself while also being useful for Ploigos integration testing scenarios as well as bases for first time users of Ploigos.

other purposes

• reduce the number of containers per pod (helps when needing to put in quotas/limits)
• make it easier for a release engineering / infra team have control over which parameters to the pipelines dev/uers can manipulate

Breaking?

Yes

Whats Breaking and why?

pipeline names

to allow for expandability without putting the work of knowing what to do on the users

pipeline params

for better validation of parammeters and give adminstrators control over which parameters users of the pipeline can set vs paramaters decided by "infra" team.

Integration Testing

• reference-quarkus-mvn - http://gitea.tssc.rht-set.com/ploigos-reference-applications/reference-quarkus-mvn/pulls/67
• reference-spring-boot-mvn-jkube - http://gitea.tssc.rht-set.com/ploigos-reference-applications/reference-spring-boot-mvn-jkube/pulls/7
• reference-undertow-mvn-s2i - http://gitea.tssc.rht-set.com/ploigos-reference-applications/reference-undertow-mvn-s2i/pulls/3
• existing container image scan - https://jenkins-jenkins.apps.tssc.rht-set.com/job/Existing%20Container%20Image%20Scan/
  • note the pipeline itself isn't green because the scanned image doesn't pass the compliance checks, but its "green" in that the pipeline completed as expected

[itewk]

@dwinchell @twling @christophermay07 @andykrohg not quite done this all yet, but whats your thoughts so far? this is more or less what we did with internal app recently and seems easier to manage from all ends. Will have to follow this up with changes for tekton and gitlab to get them closer in line.

[itewk]

@hippyod whats your thoughts on this?

[andykrohg]

Lgtm! A sound approach for handling discrepancies as new languages become supported

[hippyod]

Sorry for the stupid question, but I don't know the history behind the decisions here, and the changes you've made are pretty drastic, so it's hard for me to follow completely. I like the Serializable trick for the easy validation, but why are all the values in code, rather than a YAML or other structured config file I can just read? And setting more values in the code, using inheritance for specific configs (this feels like maybe this was for taking the Serializable trick too far maybe?), etc.?

You did ask for my thoughts, so be gentle in your castigation, please.

[itewk]

Sorry for the stupid question, but I don't know the history behind the decisions here, and the changes you've made are pretty drastic, so it's hard for me to follow completely.

@hippyod all good, not stupid, questions and speaks to our lack of useful documentation.

I like the Serializable trick for the easy validation, but why are all the values in code, rather than a YAML or other structured config file I can just read?

The intent was to stick with "Jenkins native" way(s) of solving problems. So when a developer user of the workflow uses this they create a Jenkinsfile that looks like this, https://github.com/ploigos-reference-apps/reference-quarkus-mvn/blob/feature/jenkins-params-refactor/cicd/ploigos-integration-environment/jenkins/everything/Jenkinsfile, where in they pass in the user parameters they need to.

My assumption was that it would cause more confusion to introduce another propritary yaml file with the configuration rather then just have the config passed in as funciton parameters.

BUUUUUT.....i can see the beuaty in having a config file to make clear what are the user passed in params and what are the infra set defaults. Right now the intended use is an org copies this repo into their Git manager of choice, and then updates the defaults in the UserWorkflowParams.groovy and AllWorkflowParams.groovy as well as creating / changing any of the existing workflows to their need. That means that the "infra defaults / hard coded / non changable values" are what they put in AllWorkflowParams.groovy, but thats likely not as clear as having a seperate config file.

maybe it wouldn't be to hard to add a constructor that reads a yaml file so people can do either way? I would say that maybe we do that phase 2 though.

Thoughts?

And setting more values in the code, using inheritance for specific configs (this feels like maybe this was for taking the Serializable trick too far maybe?), etc.?

Yeah. I dont disagree. on the edge of being "too sneaky". But it seemed elegent and readable enough a solution for quick parameter validation. One of the feedback we had received was release engineering / infra teams didn't want deveelopers able / concerned about most of the parameters that should be fixed once by an oversite team and forgotten.

Also the seralization trick, didn't event, picked it up from stuff from github.com/redhat-cop and Red Hat labs repos a long time ago for how they delt with programatic labeled pamareters to functions.

This was the solution came up for that and put in practice with an internal app.

Whats your thoughts on how to do the validation rather then relying on the seralization?

You did ask for my thoughts, so be gentle in your castigation, please.

I hope i didn't come off with castigation, all good questions. maybe minimally needs some more in line doc for first phase?

[itewk]

Lgtm! A sound approach for handling discrepancies as new languages become supported

@andykrohg I just added another change to refactor how the platform config works allowing an aribrary configmap and/or secret name to be provided. Should allow for more flexiability and still work with the operator with a minor update to provide the configmap and secret name rather then just True for enabling platform config.

Thoughts?

[hippyod]

@itewk So the Serialization into code as a partial validation can be good, assuming that you're not expecting extensions to be put in place by the end users (whoever that is); otherwise, this is a brittle solution. Hard for others to grasp, hard to meander through, hard to extend. Regardless, I'd use this strictly for validation, and not for defining values.

Config in code is not necessarily a "Jenkins native" practice. Jenkins shared libraries do have a resources directory, after all, and a standard manner for loading those resources. They also have a nice DSL for reading JSON and YAML. Removing configuration from code has been a best practice for longer than I can remember at this point (and that's a long time ;-) ), so moving them into external files seems the prudent, easy, and more transparent move from my perspective barring any new information. I am never a fan of complexity.

Since your map appears flat (from what I saw), a properties file seems better than YAML, too. I used those for easy leveraging of the k8s/oc functionality that translates them instantly into ConfigMaps:

kubectl create configmap some-props--from-file=some-props.properties

I mention this, because I like your other change that allows for arbitrary ConfigMap injection of configuration mentioned above. No reason end users might not want it to go the other way, too.

Oh, well, just mentioning all this as things you mull over for later improvement, as you also mentioned.

[itewk]

@hippyod so....do you think that with the way i am structing it here we coudl introduce the use of properties file as an option instead of parameters to the funciton or would it be another fundemental re-design?

[hippyod]

@itewk It's Groovy, and Groovy is groovy. There's lots of ways to skin a cat in Groovy (pardon the primitive code):

e.g. reading in java11ServicePloigosWorkflowEverything.groovy

def call(Map paramsMap) {
    //  code you already have
}

- to -

def call(def paramsSource) {
    // try/catch each file type if not a Map; `readProperties` then `readYAML` then `readJSON` then `throw`
    //    ...
    //  code you already have
}

- or, if you want to just add an alternative and allow hybrid solutions -

def call(Map paramsMap) {
    if (paramMap.CONFIG_FILES?.each {
        // try/catch each file type
        paramsMap.putAll(fileIngestedMap)
        // or replace the incoming map without reference to the `CONFIG_FILES`; whatever...
    }
    //  code you already have
}

This block of code is copy and pasted everywhere (bad smell? even has vlaidated everywhere ;-) ):

    new UserServiceWorkflowParams(paramsMap)

    // create All params from params map now the user params have been vlaidated
    params = new ServiceWorkflowParams(paramsMap)

So wrap the whole thing (whatever you decide) into a utility method that returns a Map, and spread that utility method everywhere? [NOTE: again, assuming I am reading your code, design, and intentions right, which I obviously and admittedly didn't go into too deeply. If I am off, apologies, but the general direction feels right and seems pretty trivial to add as an option.]

[dwinchell]

I get that this makes ploigos easier to maintain. It's DRY and it reduces the work for us to add a new reference pipeline, and the time for an expert to tailor a reference pipeline to an org's needs.

But ... I think it's too much. By itself it would be okay. But as an addition to all the other layers of configuration I think it raises the bar to understand ploigos too high.

If we do this, config values for a workflow that uses this and the operator can come from a total of 11 different files using 12 different schemes for passing/abstracting values:

1. SharedUserWorkflowParams (groovy extends)
2. UserExistingContainerImageScanWorkflowParams.groovy (groovy extends)
3. ExistingContainerImageScanWorkflowParams.groovy (groovy extends)
4. ConstantEnvironmentWorkflowParams (groovy implements)
5. existingContainerImageScanPloigosWorkflow.groovy
   5a. as groovy variable w/ literal assignment
   5b. as groovy variable w/ assignment from params
   5c. as direct use of params
   5d. as call to a Jenkins plugin (i.e. scm.userRemoteConfigs[0])
   5e. as an env variable set by Jenkins (i.e. WORKFLOW_WORKER_WORKSPACE_HOME_PATH)
6. Jenkinsfile (groovy constant using weird Jenkins call() scheme relying on path to groovy file)
7. config.xml (PSR)
8. config-secrets.xml (PSR & sops)
9. platform config.xml (PSR & ConfigMap)
10. platform config-secrets.xml (PSR ConfigMap & sops or k8s Secret)
11. PSR coded defaults (python)

I'm open to further discussion, but my current thinking is we should close this PR without merging. I hate to say that because I know a lot of thought and work went into it.

[itewk]

@dwinchell and i chatted off line and came to aggreement on a middle ground for this PR. updates coming in next coupel of days.