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

enable CD on bouncycastle-api #133

Merged
merged 4 commits into from
Dec 6, 2023
Merged

Conversation

ampuscas
Copy link
Contributor

@ampuscas ampuscas commented Dec 4, 2023

Testing done

Submitter checklist

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

ideally we would want to expose the bouncycastle version in the plugin version.

As 1.77 < 2.79 currently we would need some tricks

e.g. the version can be hard coded to include a 2.79. then use ${revision}${changelist} and set the bouncyastleVersion to revision

Then at some point when bounccastle release version 3 (or a version > 2.79) we can fix the mess introduced from the outset with the horrendous version number

@@ -66,8 +66,7 @@
</scm>

<properties>
<revision>2.30</revision>
<changelist>-SNAPSHOT</changelist>
<changelist>999999-SNAPSHOT</changelist>
Copy link
Member

Choose a reason for hiding this comment

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

as this is an API plugin we should not use random versions. Whilst we have already shot past the version of the bouncycastle plugin we should probably take the time to not make this worse.

You can tie the revision to the bouncycastle version (e.g. remove bouncycastleVersion and where it is used in the pom use revision then change version to 2.29.

pom.xml Outdated
@@ -34,7 +34,7 @@
</parent>

<artifactId>bouncycastle-api</artifactId>
<version>${revision}${changelist}</version>
<version>${revision}-${changelist}</version>
Copy link
Member

Choose a reason for hiding this comment

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

as is this would create versions lower than we have today.
in order to keep the cd versions newer we would need to keep a prefix like the following:

Suggested change
<version>${revision}-${changelist}</version>
<version>2.30.${revision}-${changelist}</version>

if when the bouncycastle API produces a version > 2.30 we would then be able to remove the prefix and directly use the bouncycastle version.

Copy link
Member

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

This looks good to me.

pom.xml Outdated Show resolved Hide resolved
Co-authored-by: James Nord <[email protected]>
@jglick
Copy link
Member

jglick commented Dec 4, 2023

@ampuscas
Copy link
Contributor Author

ampuscas commented Dec 6, 2023

@jtnord did you have a chance to re-review this? Could this be merged?

@jtnord jtnord merged commit 26eac94 into jenkinsci:master Dec 6, 2023
15 checks passed
@@ -34,7 +34,7 @@
</parent>

<artifactId>bouncycastle-api</artifactId>
<version>${revision}${changelist}</version>
<version>2.30.${revision}-${changelist}</version>
Copy link
Member

Choose a reason for hiding this comment

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

Why 2.30 and not simply 3?

Copy link
Member

Choose a reason for hiding this comment

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

because then we would need to wait for bouncycastle v4 rather than a v3 (or a v > 2.30). #133 (comment)

Granted we are a way of v2 let alone v3 or a v4. the earth may have imploded by the time we get either...

Copy link
Member

Choose a reason for hiding this comment

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

…if you cared about

we would then be able to remove the prefix

that is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants