-
-
Notifications
You must be signed in to change notification settings - Fork 282
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
Clean up HTTP authentication reference and Update constructors #562
base: master
Are you sure you want to change the base?
Clean up HTTP authentication reference and Update constructors #562
Conversation
2. Update constructors in JiraSite.java and JireSiteTest.java 3. Leave the `@Deprecated` annotation in case any error should occur
@@ -263,7 +263,8 @@ public JiraSite( | |||
boolean updateJiraIssueForAllStatus, | |||
@CheckForNull String groupVisibility, | |||
@CheckForNull String roleVisibility, | |||
boolean useHTTPAuth) { | |||
boolean useHTTPAuth, |
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.
Let's remove all deprecated constructors, it they are there for quite a while.
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.
Revision finished, waiting for your review!
@@ -1417,6 +1396,7 @@ static class Builder { | |||
private String groupVisibility; | |||
private String roleVisibility; | |||
private boolean useHTTPAuth; |
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.
Drop this as well
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.
Revision finished, waiting for your review!
@@ -647,7 +623,8 @@ protected Object readResolve() { | |||
updateJiraIssueForAllStatus, | |||
groupVisibility, | |||
roleVisibility, | |||
useHTTPAuth); | |||
useHTTPAuth, |
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.
@olamy I think this whole if
migration block is there for a long time as well, everyone should have already migrated those credentials so I propose to drop this whole if
as well?
Is there anything I should do toward this pull request? @rantoniuk |
I don't remember if this is serialized or not? because changing a field will break existing configuration |
@olamy correct me if I'm wrong, but from what I remember, everything that is not So we would need to:
(or alternatively again write some "migrator" that we did already in the past, but I'm just looking for a simpler solution..) When I have some time, I'll try to run some local tests to see which approach works, but maybe you already know what to suggest. |
@Deprecated
annotation in case any error should occur