Skip to content
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

[JENKINS-51183] - Introduce Integration testing flow with BOM #20

Closed
wants to merge 14 commits into from

Conversation

oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev commented May 7, 2018

Reference implementation for jenkins-infra/pipeline-library#57 . It does not do anything exactly useful outside AWS environment, and the artifact-manager-s3.enabled system property should be set for it.

On Jenkins Infra (which is on Azure) it will just run some bits with an Existing Artifact manager and new APIs. So it will smoke-tests plugins for the case when an ArtifactManager is present but not enabled. Maybe useful, but not that much.

But it is a good testing playground for @jglick's Incrementals auto-update logic (see TODO in BOM)

https://issues.jenkins-ci.org/browse/JENKINS-51183

@reviewbybees @jglick @carlossg

Jenkinsfile Outdated
@@ -1,6 +1,9 @@
if (infra.isRunningOnJenkinsInfra()) {
// ci.jenkins.io
buildPlugin(platforms: ['linux'])

// Integration tests
essentialsTest(baseDir: "src/test/it")
Copy link
Member

Choose a reason for hiding this comment

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

I guess this baseDir could just be the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. In some cases essentials.yml is located in the root (e.g. Core or Git Plugin). CC @raul-arabaolaza . Currently I use root dir as a default

Choose a reason for hiding this comment

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

I also use root dir as the default (and is the default location used by pipeline-library) but there is no strong technical reason for that, note that in any case any path can be used as is a param of the step itself

# Needed to make PCT happy
groupId: org.jenkins-ci.main
artifactId: jenkins-war
version: 2.118-artifact-manager-s3-SNAPSHOT
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to specify a version, anyway? Could the packager not just pick something arbitrary (999-SNAPSHOT), or generate a version based on the spec.core.version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Current BOM specification seems to require this field. jenkins-infra/pipeline-library#57 actually runs without it, but it will set 1.0-SNAPSHOT as a default now. Makes sense to switch to 999-SNAPSHOT as you propose

plugins:
- groupId: "org.jenkins-ci.plugins.workflow"
artifactId: "workflow-api"
version: 2.28-rc333.0675e9e9cb4c
Copy link
Member

Choose a reason for hiding this comment

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

jenkinsci/jep#92 (comment) FTR:

      branch: jglick:VirtualFile-JENKINS-49635

would be useful here in addition to the version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only for cosmetic reasons, right? When version is set, Custom WAR Packager will ignore other fields anyway

environments:
- name: aws
plugins:
#TODO: BOM does not support FileSystem REFs, so somebody needs to add BOM auto-update logic here
Copy link
Member

Choose a reason for hiding this comment

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

Uh, no, updating the BOM would not make sense here, because presumably you wish for the integration tests to be run as part of this plugin’s PR builds. So to verify some change in the plugin against these other tests, you would need to commit the change, then update the version here and commit again, before filing the PR! Does not make sense.

We could allow ${changelist} to be substituted so you would have

          version: 1.0-alpha-1${changelist}

but this would need to be updated after every MRP usage, which is still awkward. Filesystem path references could help, but it needs some exploration what they would look like. One option is

          versionFrom: ../../../.flattened-pom.xml

assuming the local process-resources phase has been run. Alternately,

          versionFrom: ../../..

where the version is inferred from the output of

mvn -Dset.changelist -Dexpression=project.version -Doutput=version help:evaluate
version=`cat version`
rm version
echo $version

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, whatever works. I could easily do it in the custom WAR packager spec, but BOM would require a whatever standardized definition in jenkinsci/jep#92

version: 2.118
plugins:
- groupId: "org.jenkins-ci.plugins.workflow"
artifactId: "workflow-api"
Copy link
Member

Choose a reason for hiding this comment

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

So this is duplicating information: the pom.xml already specifies a dependency on workflow-api, and specifies the version. We should not have that twice in the same repo.

I think this is a fundamental flaw in the idea of using a BOM in individual repos for integration tests (as opposed to the overall jenkins-infra/evergreen/configuration/essentials.yaml): there is no clear proposal for how to reconcile duplicated configuration. In jenkinsci/jep#92 (comment) @carlossg claims that bom.yml is the “source of truth” and that pom.xml would be updated by “the tooling” but I do not see how this is going to work. Is pom.xml not going to be versioned any longer? That seems impractical. So, you would need to change version numbers in both places and commit them at once? What is enforcing that they stay aligned?

To me it seems more comfortable for the POM to specify details of artifacts and their versions, and for the BOM to express additional information, if any—perhaps just delete the bom.yml altogether, since the essentials.yml here seems to have all the interesting configuration. For purposes of the custom WAR packager, you can just bundle up the jenkins.war and Jenkins plugins in Maven’s test scope. This trick works fine.

And that would also bypass the “filesystem ref” problem mentioned below.

Copy link
Member Author

@oleg-nenashev oleg-nenashev May 7, 2018

Choose a reason for hiding this comment

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

I could create a "POM to BOM" tool which dumps a YAML with all HPI dependencies in the plugin and adds them to bom.yml (would make sense to rename it to bom-template.yml then).

Copy link
Member

Choose a reason for hiding this comment

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

would make sense to rename it to bom-template.yml then

My point is that there would be nothing in such a template! essentials.yml would have all the information needed for running tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course we can do it if needed, in the most of the cases

@oleg-nenashev
Copy link
Member Author

The change requires fixes in runATH() (https://issues.jenkins-ci.org/browse/JENKINS-51306). CC @raul-arabaolaza

@raul-arabaolaza
Copy link

@oleg-nenashev jenkins-infra/pipeline-library#65 should unblock the ATH run here

version: 2.20
- groupId: "org.jenkins-ci.plugins.workflow"
artifactId: "workflow-basic-steps"
version: 2.8-rc351.c6608322f479
Copy link
Member Author

Choose a reason for hiding this comment

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

can be removed, because it comes from dependencies (need to solve upper bounds properly tho) => APPEND pom.xml deps here unless overwritten?

Copy link
Member

Choose a reason for hiding this comment

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

All of the plugins would better have their version be inherited from pom.xml. For that matter the whole section could be deleted I think and Maven test-scoped classpath used (with copyartifact added if this is in fact in active use).

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a task for that: https://issues.jenkins-ci.org/browse/JENKINS-51574
But cannot commit on any ETA

@oleg-nenashev
Copy link
Member Author

Caused by: org.eclipse.aether.transfer.ArtifactNotFoundException: Could not find artifact io.jenkins.tools.custom-war-packager:custom-war-packager-cli:jar:0.1-alpha-6-20180524.212000-3 in azure (https://repo.azure.jenkins.io/public/)

Hmm... It's either glitch or something is messed up in https://github.com/jenkins-infra/pipeline-library/blob/master/resources/settings-azure.xml . @raul-arabaolaza Should not it explicitly allow snapshots

.PHONY: all

ARTIFACT_ID = jenkins-war
VERSION = 2.121-artifact-manager-s3-SNAPSHOT
Copy link
Member

Choose a reason for hiding this comment

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

Best to avoid encoding the current core baseline version.

-e ARTIFACT_ID=artifact-manager-s3 \
-e INSTALL_BUNDLED_SNAPSHOTS=true \
jenkins/pct \
-mavenProperties jenkins-test-harness.version=2.38:jth.jenkins-war.path=/pct/jenkins.war
Copy link
Member

Choose a reason for hiding this comment

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

Should not be overriding JTH version.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://issues.jenkins-ci.org/browse/JENKINS-51575
Currently it's needed to run PCT properly against the WAR produced by CWP.

version: 2.20
- groupId: "org.jenkins-ci.plugins.workflow"
artifactId: "workflow-basic-steps"
version: 2.8-rc351.c6608322f479
Copy link
Member

Choose a reason for hiding this comment

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

All of the plugins would better have their version be inherited from pom.xml. For that matter the whole section could be deleted I think and Maven test-scoped classpath used (with copyartifact added if this is in fact in active use).

version: 2.28-rc337.8abe7c5204d9
- groupId: "org.jenkins-ci.plugins"
artifactId: "copyartifact"
version: 1.40-rc298.60da56cfabea
Copy link
Member

Choose a reason for hiding this comment

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

1.40

version: 1.14
- groupId: "org.jenkins-ci.plugins.workflow"
artifactId: "workflow-step-api"
version: 2.15-rc308.607bdefd8c6a
Copy link
Member

Choose a reason for hiding this comment

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

2.15

version: 2.8-rc351.c6608322f479
- groupId: "org.jenkins-ci.plugins.workflow"
artifactId: "workflow-support"
version: 2.18
Copy link
Member

Choose a reason for hiding this comment

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

Currently have an incremental version of this one actually.

pct:
useLocalSnapshots: false
jth:
version: 2.38
Copy link
Member

Choose a reason for hiding this comment

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

Delete.

dir: initScripts
ath:
# TODO: ATH test dependency issues
disabled: true
Copy link
Member

Choose a reason for hiding this comment

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

Why does this attribute need to exist? Why not just comment out the whole ath section?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you can do that if you want

@jglick
Copy link
Member

jglick commented Mar 27, 2019

I believe this is obsolete.

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.

3 participants