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

Revert #156 as it makes PCT sensitive to UC; instead pass groupId: in -overridenPlugins #228

Merged
merged 4 commits into from
Apr 13, 2020

Conversation

jglick
Copy link
Member

@jglick jglick commented Feb 24, 2020

Whatever #156 (& #169) was attempting to accomplish, it did it wrong—when -war is passed, the intention is that the update center should not be contacted. This misbehavior is routinely breaking bom builds. If you need information about plugins, look in the WAR file, or add some option to pass in such metadata from a deterministic file source TBD.

I tried to run the IT but it did not work:

…
+ echo 'Checking out from https://github.com/jenkinsci/artifact-manager-s3-plugin.git:artifact-manager-s3-1.6'
Checking out from https://github.com/jenkinsci/artifact-manager-s3-plugin.git:artifact-manager-s3-1.6
+ git clone https://github.com/jenkinsci/artifact-manager-s3-plugin.git
Cloning into 'artifact-manager-s3-plugin'...
fatal: unable to access 'https://github.com/jenkinsci/artifact-manager-s3-plugin.git/': Could not resolve host: github.com
make: *** [Makefile:26: test] Error 128

Anyway, unless I am missing something, this IT does not actual test local sources, only jenkins/pct:latest from the Internet, so I do not see how you could use it to verify anything.

@jglick
Copy link
Member Author

jglick commented Feb 24, 2020

Anyway if the WAR does not even include configuration-as-code then why do you need to know anything about it? Just ignore it. There should be no need for any special entry in pluginGroupIds for a plugin which you are not doing anything with. Maybe I am missing something.

@jglick
Copy link
Member Author

jglick commented Feb 24, 2020

I found

stage('Build Docker Image') {
sh 'make docker'
}
dir("src/it/war-with-plugins-test") {
def settingsXML="mvn-settings.xml"
infra.retrieveMavenSettingsFile(settingsXML)
stage('Build the custom WAR file') {
infra.runMaven(["clean", "package"])
}
stage('Run the integration test') {
sh '''docker run --rm \
-v $(pwd)/tmp/output/target/war-with-plugins-test-1.0.war:/pct/jenkins.war:ro \
-v $(pwd)/mvn-settings.xml:/pct/m2-settings.xml \
-v $(pwd)/out:/pct/out -e JDK_VERSION=8 \
-e ARTIFACT_ID=artifact-manager-s3 -e VERSION=artifact-manager-s3-1.6 \
jenkins/pct \
-overridenPlugins 'configuration-as-code=1.20'
'''
archiveArtifacts artifacts: "out/**"
sh 'cat out/pct-report.html | grep "Tests : Success"'
}
}
but confusingly it does not seem to run the make test target that you defined. Should that subdir Makefile, and its README, just be deleted if it does not work? Is there no single command you can use locally to run an IT—why is all this stuff written in Groovy?

@jglick
Copy link
Member Author

jglick commented Feb 24, 2020

And why -overridenPlugins [sic] on a plugin which is not in the WAR? I do not follow the use case here. If you care about configuration-as-code, pack it in the WAR. If you do not, omit it from the WAR and leave it at the version specified in the POM.

@jglick
Copy link
Member Author

jglick commented Feb 24, 2020

OK, finally managed to run the IT locally! (Slow because despite the attempt at caching in

# Warmup to avoid downloading the world each time
RUN git clone https://github.com/jenkinsci/plugin-compat-tester &&\
cd plugin-compat-tester && \
mvn clean package -Dmaven.test.skip=true dependency:go-offline && \
mvn clean
the build of the plugin under test downloads tons of stuff in an uncached location.) I think the root problem here is this override option.

@jglick jglick changed the title Revert #156 as it makes PCT sensitive to the Internet Revert #156 as it makes PCT sensitive to UC; instead pass groupId: in -overridenPlugins Feb 24, 2020
@jglick
Copy link
Member Author

jglick commented Apr 9, 2020

Serious, as it causes random PCT failures often enough to be a real nuisance:

Exception in thread "main" java.lang.RuntimeException: Invalid update center url : https://updates.jenkins-ci.org/update-center.json
	at org.jenkins.tools.test.PluginCompatTester.extractUpdateCenterData(PluginCompatTester.java:586)
	at org.jenkins.tools.test.PluginCompatTester.testPlugins(PluginCompatTester.java:184)
	at org.jenkins.tools.test.PluginCompatTesterCli.main(PluginCompatTesterCli.java:163)
Caused by: java.net.ConnectException: Connection timed out (Connection timed out)
	at …
	at java.net.URL.openStream(URL.java:1067)
	at org.jenkins.tools.test.PluginCompatTester.extractUpdateCenterData(PluginCompatTester.java:584)
	... 2 more

@oleg-nenashev
Copy link
Member

@jglick Sorry I have no bandwidth to process PCT issues and pull requests right now. Please ping other @jenkinsci/plugin-compat-tester-developers if urgent

@oleg-nenashev oleg-nenashev removed their request for review April 9, 2020 11:49
@jglick jglick added the bug label Apr 9, 2020
jglick added a commit to jglick/bom that referenced this pull request Apr 9, 2020
@jglick
Copy link
Member Author

jglick commented Apr 9, 2020

java.lang.RuntimeException: Invalid update center url : https://updates.jenkins.io/current/update-center.json
	at org.jenkins.tools.test.PluginCompatTesterTest.testWithUrl(PluginCompatTesterTest.java:90)
Caused by: java.net.ConnectException: Connection timed out: connect
	at org.jenkins.tools.test.PluginCompatTesterTest.testWithUrl(PluginCompatTesterTest.java:90)

seems like a flake

Copy link
Contributor

@raul-arabaolaza raul-arabaolaza left a comment

Choose a reason for hiding this comment

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

LGTM but I would like to perform some in depth testing on Monday (out of the office now) to be sure is not breaking any of our pipelines, I will approve then or further comment

Copy link
Contributor

@raul-arabaolaza raul-arabaolaza left a comment

Choose a reason for hiding this comment

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

Initial tests work so approving, if some corner case arises we can fix later.

I am going to close, reopen to trigger again the PR builder to be sure the test failure is just flakiness in the UC

@raul-arabaolaza raul-arabaolaza merged commit 8c55a1f into jenkinsci:master Apr 13, 2020
@jglick jglick deleted the war-no-uc branch April 13, 2020 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants