-
Notifications
You must be signed in to change notification settings - Fork 66
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
Conversation
Jenkinsfile
Outdated
@@ -1,6 +1,9 @@ | |||
if (infra.isRunningOnJenkinsInfra()) { | |||
// ci.jenkins.io | |||
buildPlugin(platforms: ['linux']) | |||
|
|||
// Integration tests | |||
essentialsTest(baseDir: "src/test/it") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
src/test/it/bom.yml
Outdated
# Needed to make PCT happy | ||
groupId: org.jenkins-ci.main | ||
artifactId: jenkins-war | ||
version: 2.118-artifact-manager-s3-SNAPSHOT |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
src/test/it/bom.yml
Outdated
plugins: | ||
- groupId: "org.jenkins-ci.plugins.workflow" | ||
artifactId: "workflow-api" | ||
version: 2.28-rc333.0675e9e9cb4c |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
src/test/it/bom.yml
Outdated
environments: | ||
- name: aws | ||
plugins: | ||
#TODO: BOM does not support FileSystem REFs, so somebody needs to add BOM auto-update logic here |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
src/test/it/bom.yml
Outdated
version: 2.118 | ||
plugins: | ||
- groupId: "org.jenkins-ci.plugins.workflow" | ||
artifactId: "workflow-api" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
The change requires fixes in |
@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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
I believe this is obsolete. |
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