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

Update plugin parent POM and BOM #232

Closed
wants to merge 1 commit into from
Closed

Update plugin parent POM and BOM #232

wants to merge 1 commit into from

Conversation

basil
Copy link
Member

@basil basil commented Apr 21, 2022

No description provided.

@basil basil requested a review from a team as a code owner April 21, 2022 02:05
@basil
Copy link
Member Author

basil commented Apr 21, 2022

@jenkinsci/cloudbees-folder-plugin-developers A release of this would facilitate Java 17 PCT testing.

<artifactId>bom-2.277.x</artifactId>
<version>984.vb5eaac999a7e</version>
<artifactId>bom-2.289.x</artifactId>
<version>1289.v5c4b_1c43511b_</version>
Copy link
Member

@jtnord jtnord Apr 21, 2022

Choose a reason for hiding this comment

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

@basil I am generally against bumping bom versions unless it is needed to pick up a fix specifically needed for the plugin as it causes issues when upgrading for a fix. as the PCT will test with newer versions and this plugin is part of the bom - is this strictly needed give the 984 version is availabe for the 2.289 bom?

Suggested change
<version>1289.v5c4b_1c43511b_</version>
<version>984.vb5eaac999a7</version>

Copy link
Member

Choose a reason for hiding this comment

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

it causes issues when upgrading for a fix

Can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

example
plugin A uses the bom, has dependencies on B C D E.....

bom update is merged to A. (uninteresting so no CD release is made)

bug fix is made to A (new release is made by CD)

to get the bug fix you now need to upgrade A B C D and E....

this causes issues as B C D E whilst they may have past unit tests, they may contain bugs that are not found.
it also makes it harder to get users to test fixes as they need to upgrade way more than just a plugin.

regressions do to the upgrade are harder to pin down, git bisect will see just one commit upgrading the world, so you need to resort to non standard scripts to get and test the versions of the bom between last release and now.

TL;DR if the plugin does not need an upgrade of B C D or E then there is no reason to upgrade the bom especially if the plugin is itself part of the bom as the compatability testing will happen in the BOM (at least binary compatability, not currently source, but if that is cared about it is a simple thing to do)

Copy link
Member

@jtnord jtnord Apr 21, 2022

Choose a reason for hiding this comment

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

for this plugin it only affect credentials and its transitive deps (others are test scope), but generally having special case plugins is root to pain.

Copy link
Member

Choose a reason for hiding this comment

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

B C D E whilst they may have past unit tests, they may contain bugs that are not found

Of course they may, but these are long since on the update center in users’ hands, which would be the more important consideration by far.

regressions do to the upgrade are harder to pin down

You mean regressions in tests? While it is possible for a BOM update to regress production functionality, that would be rare.

harder to get users to test fixes

Regardless of how or even if you are using the BOM, in general if you are asking someone to test a particular fix in situ, it is advisable to ask which version they are currently running, then create a draft PR rooted at that version (or the subsequent commit, if still using MRP) backporting the fix with git cherry-pick -x -m1, then point them to the resulting incremental build.

Copy link
Member

Choose a reason for hiding this comment

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

Regardless of how or even if you are using the BOM, in general if you are asking someone to test a particular fix in situ,

this is creating extra work when they are already on the latest release.

for users how are upgrading updating the bom in the plugin gets them nothing - it is a developer thing.
for users that are not upgrading biumping the bom unesescarily is creating more work for the developer and the user.

Copy link
Member

@jtnord jtnord Apr 21, 2022

Choose a reason for hiding this comment

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

but these are long since on the update center in users’ hands, which would be the more important consideration by far.

isn;t the bom updated and released by CD and dependabot so long since is actually a day? (or however long it takes to run the pipeline)

Copy link
Member

Choose a reason for hiding this comment

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

isn’t the bom updated and released by CD

Dependabot bumps to bom are marked dependencies so they are not automatically released. Typically we only bother to cut a release when adding new plugins, or doing upgrades which fix some sort of logistical POM issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

as the PCT will test with newer versions

[…]

especially if the plugin is itself part of the bom as the compatability testing will happen in the BOM

jenkinsci/bom#821

Copy link
Member

Choose a reason for hiding this comment

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

(of course we expect that to be fixed at some point)

@basil
Copy link
Member Author

basil commented Apr 22, 2022

Is there a reason #232 (review) has not been dismissed? As demonstrated in #232 (comment) the premise of the review was flawed; therefore, the conclusion is flawed.

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.

(FWIW)

@jglick jglick requested a review from jtnord April 22, 2022 17:58
@basil
Copy link
Member Author

basil commented Apr 25, 2022

@jtnord ?

@jtnord
Copy link
Member

jtnord commented Apr 25, 2022

Is there a reason #232 (review) has not been dismissed? As demonstrated in #232 (comment) the premise of the review was flawed; therefore, the conclusion is flawed.

because there is absolutely nothing wrong with #232 (comment) ? the only part that is flawed is the assumption that the BOM is actually testing things in combination. as it is not - this makes me more wary to pickup boms as there has not been the testing that I explicitly thought there had been :and it would push users to upgrade to these versions which is more risky than expected -o

@basil can you please provide an answer the original question?

is this strictly needed give the 984 version is availabe for the 2.289 bom?

@basil basil closed this Apr 25, 2022
@jglick
Copy link
Member

jglick commented Apr 25, 2022

If the BOM update is controversial, can the rest be refiled?

@basil
Copy link
Member Author

basil commented Apr 25, 2022

It can. I have no plans to do so.

@jtnord
Copy link
Member

jtnord commented Apr 25, 2022

If the BOM update is controversial, can the rest be refiled?

I left an inline suggestion with the original comment I have no access to Basil's fork to apply it

@basil
Copy link
Member Author

basil commented Apr 25, 2022

I do not accept your suggestion.

@basil
Copy link
Member Author

basil commented Apr 25, 2022

Since I wrote the above, James contacted me to explain that dependency updates sometimes cause him pain when dealing with ATH failures. Is an ATH maintainer who is experiencing pain caused by dependency updates justified in blocking a pull request that updates dependencies and could possibly cause them more pain? Not if it is uncertain that the pull request will actually cause the ATH maintainer pain. In fact, in this case the BOM maintainer could be justified in updating dependencies in order to mitigate their own pain resulting from jenkinsci/bom#821. So here we have a conflict between two maintainers, each of whom is justified in their actions from their own perspective but neither of whom deserves the conflict. In order to eliminate the conflict at its root and optimize globally rather than locally, we need to shift testing left in the software delivery lifecycle. I have started a mailing list thread to discuss this in a broader forum.

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

Successfully merging this pull request may close these issues.

3 participants