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

[FIXED JENKINS-44809] Clean up existing job property handling. #161

Merged
merged 3 commits into from Jun 12, 2017

Conversation

Projects
None yet
5 participants
@abayer
Copy link
Member

commented Jun 11, 2017

  • JENKINS issue(s):
  • Description:
    • Two aspects - stop re-adding existing job properties defined outside of the Jenkinsfile, since they can't have changed, and remove all existing instances of a job property class defined inside the Jenkinsfile before we add the new instance, to avoid duplication.
  • Documentation changes:
    • n/a
  • Users/aliases to notify:

@abayer abayer requested a review from jglick Jun 11, 2017

@abayer abayer added this to the 1.1.6 milestone Jun 11, 2017

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2017

@jglick - Any suggestions on how to reproduce the original bug would be appreciated, but I think it may be impossible to do so in a unit test...

@reviewbybees

This comment has been minimized.

Copy link

commented Jun 11, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2017

Ok, thanks to a later comment I know for sure what the root problem is (duplicate JobProperty instances) so testing is a bit easier.

@abayer abayer force-pushed the abayer:jenkins-44809 branch from 18263f4 to 9d514b0 Jun 11, 2017

abayer added some commits Jun 11, 2017

[FIXED JENKINS-44809] Clean up existing job property handling.
Two aspects - stop re-adding existing job properties defined outside
of the Jenkinsfile, since they can't have changed, and remove all
existing instances of a job property class defined inside the
Jenkinsfile before we add the new instance, to avoid duplication.

@abayer abayer requested a review from rsandell Jun 11, 2017

@rsandell
Copy link
Member

left a comment

🐝

@oleg-nenashev
Copy link
Member

left a comment

Looks good to me 🐝

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2017

@jglick FYI, I'm going to have two follow-ups to this - moving the non-triggers/parameters logic into properties, and then moving this to use that (with some special sauce for triggers/parameters) instead of its own implementation. But I wanted to get this bug fixed first.

@jglick

jglick approved these changes Jun 12, 2017

Copy link
Member

left a comment

Not exactly sure what this is about, but it has a test so OK.

// instance of the class that we've seen so far. Ideally we'd be ignoring it completely, but due to
// JENKINS-44809, we've created situations where tons of duplicate job property instances exist,
// which need to be nuked, so go through normal cleanup.
if (!jobPropertiesToApply.any { p.class.isInstance(it) }) {

This comment has been minimized.

Copy link
@jglick

jglick Jun 12, 2017

Member
p.descriptor == it.descriptor

is what I think you mean.

This comment has been minimized.

Copy link
@abayer

abayer Jun 12, 2017

Author Member

Ah, yeah, that should work. Trying it.

This comment has been minimized.

Copy link
@abayer

abayer Jun 12, 2017

Author Member

And done.

// Remove the existing instance(s) of the property class before we add the new one. We're looping and
// removing multiple to deal with the results of JENKINS-44809.
while (j.getProperty(p.class) != null) {
j.removeProperty(p.class)

This comment has been minimized.

Copy link
@jglick

jglick Jun 12, 2017

Member

OK for now though these methods in Job should actually be deprecated and replaced by ones which operate on Descriptor.id.

This comment has been minimized.

Copy link
@abayer

abayer Jun 12, 2017

Author Member

Agreed wholeheartedly. =)

This comment has been minimized.

Copy link
@abayer

abayer Jun 12, 2017

Author Member

...and they should probably remove all instances with the given Descriptor.id rather than just the first...

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2017

Merging/releasing once the tests pass, just to be safe.

@abayer abayer merged commit 3f6a39c into jenkinsci:master Jun 12, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.