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 authentication-tokens to the managed set #2189

Merged
merged 4 commits into from
Jun 28, 2023

Conversation

MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Jun 16, 2023

Add authentication-tokens plugin to managed set

Use latest release for all versions now included in bom.

Fixes #2095

Testing done

Confirmed that tests work with the following commands:

$ mvn -P 2.375.x --pl sample-plugin verify
$ mvn -P 2.387.x --pl sample-plugin verify
$ mvn -P 2.401.x --pl sample-plugin verify
$ mvn -P weekly  --pl sample-plugin verify
$ PLUGINS=authentication-tokens bash local-test.sh

Submitter checklist

@MarkEWaite MarkEWaite requested a review from a team as a code owner June 16, 2023 22:16
@MarkEWaite MarkEWaite added the developer Pull requests that merits a release for plugin developers label Jun 16, 2023
@jglick
Copy link
Member

jglick commented Jun 16, 2023

Do you not want to full-test this?

@MarkEWaite MarkEWaite added the full-test Test all LTS lines in this PR and do not halt upon first error. label Jun 16, 2023
@MarkEWaite
Copy link
Contributor Author

Do you not want to full-test this?

Thanks for the pointer. Yes, I've labeled it with full-test and started a new run. I expect it to pass the full-test. Once it has passed that, I'll merge it and plan to release the master branch.

MarkEWaite added a commit to MarkEWaite/plugin-compat-tester that referenced this pull request Jun 17, 2023
jenkinsci/bom#2095 requests the addition of
authentication tokens to the managed set in the plugin bill of materials.

jenkinsci/bom#2189 implements that but shows
that the addition fails because authentication-tokens plugin 1.4 needs
to be pinned for Jenkins 2.375.x and authentication-tokens plugin 1.4
lists its SCM location with a git:// protocol URL instead of an https://
protocol URL.

Use the same workaround that is used for other older plugins.

This can be removed once the 2.375.x line is no longer needed in the
plugin bill of materials.
@MarkEWaite
Copy link
Contributor Author

MarkEWaite commented Jun 17, 2023

Will require a new release of the plugin compatibility tester before this can be merged. Jenkins 2.375.x needs authentication tokens plugin 1.4 but that release includes the scm URL with a git:// protocol. The git:// protocol is now rejected by GitHub. That prevents checkout of the authentication token plugin 1.4 sources, so the testing fails.

Moving this to draft status, since it can't be merged until after a new release of PCT.

Needs merge and release of

@MarkEWaite MarkEWaite marked this pull request as draft June 17, 2023 02:27
@MarkEWaite MarkEWaite marked this pull request as ready for review June 27, 2023 03:25
@timja timja enabled auto-merge (squash) June 27, 2023 07:51
Comment on lines +220 to +224
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>authentication-tokens</artifactId>
<scope>test</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

OK, though normally we would prefer to include it implicitly via some other plugin with visible features.

Ideally we would use dependency:analyze or similar to verify that this POM includes a minimal set of dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I had not considered that technique. I think that means that the final conclusion would be to resolve the issue:

with pull request

and include the Docker Commons API plugin in the sample plugin. It depends on authentication-tokens.

Since docker-commons is an api plugin without visible features, should we consider adding Docker Pipeline to the managed set (110 000 installations) ?

Copy link
Member

Choose a reason for hiding this comment

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

Probably yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we would use dependency:analyze or similar to verify that this POM includes a minimal set of dependencies

I'm not skilled with maven, but I ran mvn --pl sample-plugin dependency:analyze and it reported many declared but unused dependencies. I tried removing the first one in the list, jsch, and that caused mvn --pl sample-plugin verify to fail with the report that jsch is not listed in the test classpath of the sample plugin.

However, the caffeine-api plugin is not mentioned in the sample-plugin pom file yet is mentioned in the bom-weekly. Apparently some plugins do not need to be listed in sample-plugin/pom.xml but it is not apparent to me which must be listed and which can be skipped.

Clearly I have much more to learn about maven and dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

the caffeine-api plugin is not mentioned in the sample-plugin pom file yet is mentioned in the bom-weekly. Apparently some plugins do not need to be listed in sample-plugin/pom.xml but it is not apparent to me which must be listed and which can be skipped.

Those which would be dependencies of other explicitly listed plugins can be omitted, and preferably should (especially in the case of API-only plugins). Whether this is amenable to mechanical enforcement is another question.

I ran … dependency:analyze and it reported many declared but unused dependencies

Unfortunately it is designed for plain old Java programs and there are numerous subtleties specific to Jenkins plugins.

@MarkEWaite MarkEWaite changed the title Add authentication-tokens plugin to managed set Add authentication-tokens to managed set Jun 28, 2023
@MarkEWaite MarkEWaite changed the title Add authentication-tokens to managed set Add authentication-tokens to the managed set Jun 28, 2023
@jglick jglick added enhancement New feature or request and removed developer Pull requests that merits a release for plugin developers labels Jun 28, 2023
@timja timja merged commit 189b671 into jenkinsci:master Jun 28, 2023
@MarkEWaite MarkEWaite deleted the add-authentication-tokens branch July 26, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request full-test Test all LTS lines in this PR and do not halt upon first error.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Authentication Tokens to the managed set
4 participants