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

Add pipeline-model-definition plugin #179

Merged
merged 21 commits into from Sep 30, 2020
Merged

Add pipeline-model-definition plugin #179

merged 21 commits into from Sep 30, 2020

Conversation

chriskilding
Copy link
Contributor

@chriskilding chriskilding commented Jan 27, 2020

Add pipeline-model-definition plugin to the BOM. Add a declarative pipeline test to sample-plugin.

</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>docker-workflow</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

I would like to do JENKINS-58732 BTW.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's ok - and indeed would let us knock a few transitive dependencies off what is quite a big change here. We can revisit this when that PR is merged

Copy link
Member

Choose a reason for hiding this comment

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

Also the split would reduce the number of tests run in pipeline-model-definition, which is currently gigantic, even slower than workflow-cps, thus becoming the bottleneck for the bom parallel run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that jenkinsci/pipeline-model-definition-plugin#373 was merged earlier today - let me know when the next version is released, and I'll re-work this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Actually it is going to take longer than that, for purposes of this PR. See rollout plan in jenkinsci/docker-workflow-plugin#202. I do not think you need to block this PR on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to avoid wrangling the icon-shim-plugin's inconsistent tags, which I believe docker-workflow was pulling in, by waiting. Is that unblocked at this point, or is is it still present?

Copy link
Member

Choose a reason for hiding this comment

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

Afraid I know nothing about this icon-shim issue.

@chriskilding
Copy link
Contributor Author

While we're at it, any idea why the PCT step fails to check out the icon-shim plugin from source? I looked in its Git repo and there certainly seems to be a version 1.0.3.

error: pathspec 'icon-shim-1.0.3' did not match any file(s) known to git

@jglick
Copy link
Member

jglick commented Jan 28, 2020

any idea why the PCT step fails to check out the icon-shim plugin from source?

Not offhand. Something nonstandard with the metadata or Git layout perhaps.

@chriskilding
Copy link
Contributor Author

chriskilding commented Jan 29, 2020

Looks like the icon-shim plugin is only being pulled in by docker-commons, so if you do the other PR first then this problem will also go away. (Well until we decide to put icon-shim in the POM for other reasons.)

+- org.jenkins-ci.plugins:docker-workflow:jar
|  \- org.jenkins-ci.plugins:docker-commons:jar
|     +- org.jenkins-ci.plugins:authentication-tokens:jar
|     \- org.jenkins-ci.plugins.icon-shim:icon-shim:jar

@chriskilding
Copy link
Contributor Author

But if we were interested in fixing it, the answer is as you said, non-standard Git layout.

Most plugins either have a tag naming convention of [plugin]-[version] or [version]. But the icon-shim plugin is non-standard, so the PCT checkout won't work:

$ git tag
icon-shim-1.0.0
icon-shim-pom-1.0.1
icon-shim-pom-1.0.2
icon-shim-pom-1.0.3
icon-shim-pom-1.0.4
...

@jetersen
Copy link
Member

@jglick
Copy link
Member

jglick commented Mar 6, 2020

the icon-shim plugin is non-standard, so the PCT checkout won't work

Supposed to have been taken care of by jenkinsci/plugin-compat-tester#181 without any tricks.

@chriskilding
Copy link
Contributor Author

After some more dependency twiddling it appears docker-commons imports some more troublesome dependencies - notably authentication-tokens which fails PCT on its InjectedTest:

[ERROR] InjectedTest.initializationError(InjectedTest)
[ERROR]   Run 1: InjectedTest.suite:15 » NoClassDefFound hudson/slaves/CommandLauncher

That plugin hasn't been updated since last year and still uses parent 3.42 / Jenkins 2.60.3, so bringing it up to date may not be trivial. Is it worth it? Would it be easier to wait for the Docker dependencies to be removed from pipeline-model-definition.

@jglick
Copy link
Member

jglick commented Mar 10, 2020

If there is an error in authentication-tokens it should be simple to solve. Just updating the parent POM will generally suffice to pull in jenkinsci/jenkins-test-harness#79.

@jglick
Copy link
Member

jglick commented Mar 10, 2020

wait for the Docker dependencies to be removed from pipeline-model-definition

Or jenkinsci/pipeline-model-definition-plugin#383 might suffice.

@chriskilding
Copy link
Contributor Author

Have opened jenkinsci/authentication-tokens-plugin#7 for this

@chriskilding
Copy link
Contributor Author

It appears that even when the docker workflow dep is marked optional, it's still dragged in to the analysis with its own dependencies. I've filed jenkinsci/pipeline-model-definition-plugin#391 to do the honours

@jglick
Copy link
Member

jglick commented Jun 23, 2020

jenkinsci/authentication-tokens-plugin#7 is released as 1.4.

@chriskilding chriskilding marked this pull request as ready for review June 24, 2020 09:48
@jetersen
Copy link
Member

Currently has some conflicts

@chriskilding
Copy link
Contributor Author

also seems to have lost the CI trigger. I tried unmarking as draft to see if that would make a difference.

@jetersen
Copy link
Member

it won't trigger CI with conflicts

@chriskilding
Copy link
Contributor Author

Looks like a lot more tests are able to pass now.

The next blocker is java.lang.NoClassDefFoundError: hudson/slaves/CommandLauncher in the pipeline-stage-step PCT. The curious thing is, command-launcher-plugin (which contains that class) is available on the classpath, at least as it appears in my local IDE checkout.

@timja
Copy link
Member

timja commented Jun 24, 2020

Weird it seems to be included:

https://github.com/jenkinsci/command-launcher-plugin/blob/master/src/main/java/hudson/slaves/CommandLauncher.java

https://github.com/chriskilding/bom/blob/add-pipeline-model-definition-plugin/bom-latest/pom.xml#L136

Local IDE checkout doesn't really represent what's actually run, best to run the PCT locally with the local-test script or my grabbing the command out that's used in the Jenkins job.

Basically PCT does some trickery...

bom-latest/pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Getting close!

bom-latest/pom.xml Outdated Show resolved Hide resolved
bom-latest/pom.xml Outdated Show resolved Hide resolved
bom-latest/pom.xml Outdated Show resolved Hide resolved
sample-plugin/pom.xml Outdated Show resolved Hide resolved
bom-latest/pom.xml Show resolved Hide resolved
" stages {",
" stage('Example') {",
" steps {",
" echo 'Hello world'",
Copy link
Member

Choose a reason for hiding this comment

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

May as well

Suggested change
" echo 'Hello world'",
" example(x: 'some value')",

and for that matter just move this to ExampleStepTest.

@chriskilding
Copy link
Contributor Author

More things working now.

The basics test in org.jenkinsci.plugins.workflow.support.steps.StageStepTest still fails, which I think is the next bottleneck in the graph.

@chriskilding
Copy link
Contributor Author

FWIW the pipeline that the test constructs is unusual in that the stages don't wrap their contents, and the build steps (chiefly echo) are not wrapped in step {} either:

stage(name: 'A', concurrency: 2);
echo('in A');
semaphore('B');
stage(name: 'B', concurrency: 1);
echo('in B');
semaphore('X');
echo('done')

I believe I saw a warning recently that this kind of stage usage might be deprecated, though can't recall where exactly I saw this. And Jesse has just pushed the first pipeline-stage-step release in 2/3 years. Is the test no longer using valid pipeline DSL?

@jglick
Copy link
Member

jglick commented Jul 20, 2020

I would again propose #179 (comment)

@chriskilding
Copy link
Contributor Author

chriskilding commented Jul 20, 2020

I've incrementally removed dependencies as we've gone along, so we're looking at something near the minimal set already:

  • pipeline-model-definition (and its multimodule artifacts)

Plus three that the sample project requires to be present in order to build - if omitted we get the "Plugin dependency on [plugin] is not from dependencyManagement" error:

  • jackson2-api
  • workflow-multibranch
  • pipeline-stage-step

@jglick
Copy link
Member

jglick commented Jul 20, 2020

jackson2-api, workflow-multibranch, and pipeline-stage-step could all be split off into their own PRs. (Cherry-picking b489a73, assuming it works as intended.)

@jglick
Copy link
Member

jglick commented Jul 20, 2020

Time permitting, we should also be looking to add some other plugins mentioned in jenkinsci/workflow-aggregator-plugin#36.

@jglick
Copy link
Member

jglick commented Jul 20, 2020

I am afraid there seem to be a number of plugins already in the BOM which did not pass PCT, something that needs to be fixed before adding new plugins. Will file a new PR to track this.

jglick added a commit to jglick/bom that referenced this pull request Jul 20, 2020
…ill leave no test files behind; really better for PCT to set -Dmaven.test.failure.ignore

(cherry picked from commit 7c271c3)
@chriskilding
Copy link
Contributor Author

workflow-multibranch-plugin proposed in #280

@chriskilding
Copy link
Contributor Author

pipeline-stage-step proposed in #281

@chriskilding
Copy link
Contributor Author

jackson2-api proposed in #282

@jetersen
Copy link
Member

Are we ready to move forward with this one?

@chriskilding
Copy link
Contributor Author

chriskilding commented Sep 30, 2020

gotta rebase and tidy up, but yep

@jetersen
Copy link
Member

jetersen commented Sep 30, 2020

We are squash merging so no need for rebase 😁

@chriskilding
Copy link
Contributor Author

Hmm, got the same failure on test_submitters() in InputStepTest in pipeline-input-step-plugin (403 login forbidden) as I did in the workflow-multibranch PR. It passed after a rebuild in that PR, so suspect there might be some flakiness there.

@chriskilding chriskilding marked this pull request as ready for review September 30, 2020 18:12
@chriskilding
Copy link
Contributor Author

Ok definitely a couple of flaky tests in there, selfTestLibraries in SCMRetrieverTest timed out and test_submitters in InputStepTest couldn't log in.

Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

Changes look good to me however the flaky tests are worrisome 😓

@jglick jglick added the enhancement New feature or request label Sep 30, 2020
@jglick
Copy link
Member

jglick commented Sep 30, 2020

a couple of flaky tests in there

Both of those sound familiar from other PRs. Will have to dig into them at some point. Likely unrelated to this PR.

@jglick jglick merged commit 47dc496 into jenkinsci:master Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants