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

[JENKINS-49574] JEP-200 compatibility #46

Merged
merged 14 commits into from Feb 28, 2018

Conversation

@jglick
Copy link
Member

commented Feb 19, 2018

Amends #45 to reproduce the actual problem with changelogs in a test, and fix it properly.

@reviewbybees

@jglick jglick requested review from fredg02 and oleg-nenashev Feb 19, 2018

@oleg-nenashev
Copy link
Member

left a comment

It makes the implementation nicer, but it breaks binary compatibility and potentially drops the persisted data with undefined impact on the behavior. Not sure we want that, but it's up to @fredg02 .

My PR in #45 is a "conservative" one


private String user;
private String msg;
private final List<File> files = new ArrayList<File>();
private Calendar changeDate;
private long changeDate;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 19, 2018

Member

Should the field be renamed? IIUC in the current state it may drop the historical data if it is persisted somewhere. With renamed field ti would be an Old Data Monitor instead of crappy error message at least

This comment has been minimized.

Copy link
@jglick

jglick Feb 19, 2018

Author Member

The field is truly private. It is never saved or read via XStream.

This comment has been minimized.

Copy link
@jglick

jglick Feb 19, 2018

Author Member

(It is visible in the serial form for Remoting, of course, but that has no diachronic implications.)

private CvsRepository repository;

/**
* Returns true if all the fields that are supposed to be non-null is
* present. This is used to make sure the XML file was correct.
*/
public boolean isComplete() {
return changeDate != null && msg != null;
return changeDate != 0 && msg != null;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 19, 2018

Member

"0" is a default value if the field is not present. It may cause the "complete" entries to switch to "incomplete" aftr reloading from the disk. Not sure what would be the impact, but it smells

This comment has been minimized.

Copy link
@jglick

jglick Feb 19, 2018

Author Member

Not sure what you think the issue would be. The logic is analogous to when the field was of a reference type. It is either initialized or not.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

breaks binary compatibility

Yes. Could be restored if anyone really cares.

and potentially drops the persisted data

Only for very old build records for which it is unlikely anyone wants a changelog any more.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Feb 19, 2018

Let's see what @fredg02 says

@jglick jglick requested a review from mc1arke Feb 28, 2018

@jglick jglick referenced this pull request Feb 28, 2018
4 of 4 tasks complete

@jglick jglick merged commit b06434d into jenkinsci:master Feb 28, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@jglick jglick deleted the jglick:JEP-200-JENKINS-49574 branch Feb 28, 2018

@jglick jglick referenced this pull request Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.