apheleia-project / jbs-build-issues

0 stars 0 forks source link

JBS Build Info Variable Substitutions #13

Closed rnc closed 4 months ago

rnc commented 5 months ago

Currently within a build-info build.yaml script we can have eg

additionalArgs:
  - "-Pkotlin.build.isObsoleteJdkOverrideEnabled=true"
  - "-Pmaven.repository.mirror=$(CACHE_URL)"
  - "-PdeployVersion=$(PROJECT_VERSION)"
postBuildScript: |
  mvn -DnewVersion=$(PROJECT_VERSION) -DgenerateBackupPoms=false -DprocessAllModules=true -f libraries/pom.xml versions:set
  mvn -V -B -e -s $(workspaces.build-settings.path)/settings.xml -DaltDeploymentRepository=local::file:$(workspaces.source.path)/artifacts -DskipTests -DuseJBSDeployed -f libraries/pom.xml package org.apache.maven.plugins:maven-deploy-plugin:3.1.1:deploy

Within additionalArgs I think the variable substitutions are handled by Tekton as part of https://tekton.dev/docs/pipelines/variables/#fields-that-accept-variable-substitutions ?

The problem is that the fields in postBuildScript are not handled in the same way. The above configuration leads to

mvn -DnewVersion=$(PROJECT_VERSION) -DgenerateBackupPoms=false -DprocessAllModules=true -f libraries/pom.xml versions:set
mvn -V -B -e -s /workspace/build-settings/settings.xml -DaltDeploymentRepository=local::file:/workspace/source/artifacts -DskipTests -DuseJBSDeployed -f libraries/pom.xml package org.apache.maven.plugins:maven-deploy-plugin:3.1.1:deploy

Obviously PROJECT_VERSION has not been changed. It is set in the pod as an environment variable so if it was $PROJECT_VERSION this would work. However thats not very consistent from the user point of view.

Its not clear what has changed -DaltDeploymentRepository=local::file:$(workspaces.source.path)/artifacts ? Is this tekton as well?

This affects https://github.com/redhat-appstudio/jvm-build-data/pull/171

rnc commented 5 months ago

Using $(params.PROJECT_VERSION) does get substituted in the postBuildScript section. It appear to also work in the additionalArgs section ( https://github.com/rnc/jvm-build-data/commit/3f9defcb79c251001b4740d0fc062fc5b5d004fb )

echo "Running Gradle command with arguments: $@"
Running Gradle command with arguments: assemble publishToMavenLocal -DdisableTests -Pkotlin.build.isObsoleteJdkOverrideEnabled=true -Pmaven.repository.mirror=https://jvm-build-workspace-artifact-cache-tls.test-jvm-namespace.svc.cluster.local/v2/cache/rebuild-all,intellij-releases,intellij-dependencies/1631709680000 -PdeployVersion=1.5.31

@dwalluck @stuartwdouglas Should we update the recommendation to use that format or add special processing for the buildScript in createPipelineSpec ?

dwalluck commented 5 months ago

$() doesn't denote a shell variable (that would be ${}, so I assume that these are all meant to be params.

stuartwdouglas commented 5 months ago

We should make all of these env variables, and not have any references to tekton substitutiuons such as params or workspace paths.

We should document in the repo which env variables are provided.

rnc commented 5 months ago

Using e.g. $PROJECT_VERSION in additionalArgs just results in that exact string ending up in the build pod not the actual value of the project version variable. Using e.g. $PROJECT_VERSION in postBuildScript / preBuildScript does work - but that is back to the inconsistent problem I mentioned.

Of the special variables that are not environment variables - there are only a couple - workspaces.build-settings.path , workspaces.source.path. All the rest are environment variables - but that isn't the issue, its the substitution that is.

I do think its simplest (for now, we could do larger changes later) to simply state in the jvm-build-data README that:

image

rnc commented 4 months ago

For now, I've used the format of params.XXX and updated the jvm-build-data README to reflect that in the above issue.