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
Make TimeZoneProperty compatible with JCasc #4557
Conversation
@@ -39,11 +39,6 @@ private TimeZoneProperty() { | |||
this(null); | |||
} | |||
|
|||
@Override | |||
public void save() throws IOException { | |||
user.save(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caused null pointers in jcasc as there's a transient user reference set on the property that can't be set through regular databinding,
I haven't seen this save pattern on any other user property and it works fine without it
I've tested from the UI and restarted jenkins and it is all fine, (and through jcasc)
note that this class is restricted no external use so evolving it is fine
class references:
https://github.com/search?q=org%3Ajenkinsci+TimeZoneProperty&type=Code
I would like to review it before it is merged. Removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is @Restricted
I'm a bit more comfortable in breaking the compatibility of removing the interface implementation.
Ping @oleg-nenashev could you do your review please? |
on it, sorry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saveable
was added in e5ae081 - first commit in #4113 by @silent-snowman . There are some other user properties which are saveable (e.g. PaneStatusProperties), but the majority of properties do not have such interface.
For me there are only 2 use-cases to have the UserProperty Saveable :
- There is extra UI / REST API / CLI command which updates the property and saves it
- There are
SaveableListener
s watching for events
None of that applies to the TimeZoneProperty, I was unable to find any external usages which would actually depend on the object being saveable. Hence I approve it.
Thanks for the reviews all, Let's merge this tomorrow if no negative feedback |
|
remove "myView", you'll likely also want to remove "apiToken" and I'm not sure if "timeZone" will work when empty like that (it may) |
@Y0lan Please ask for help in chat or on the users mailing list, or report bugs in the issue tracker. |
I was having a similar issue and removing "myView", "apiToken" and "timeZone" made things start working. |
See jenkinsci/configuration-as-code-plugin#1309 jenkinsci/configuration-as-code-plugin#1254
Example yaml:
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgradeDesired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are correctupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate