-
Notifications
You must be signed in to change notification settings - Fork 73
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
Configurable Session Token Duration #48
Configurable Session Token Duration #48
Conversation
1d56305
to
492dc31
Compare
Ping @andresrc Any feedback on this? Hoping to get it through soon if we can |
src/main/java/com/cloudbees/jenkins/plugins/awscredentials/AWSCredentialsImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/cloudbees/jenkins/plugins/awscredentials/AWSCredentialsImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/cloudbees/jenkins/plugins/awscredentials/AWSCredentialsImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/cloudbees/jenkins/plugins/awscredentials/AWSCredentialsImpl.java
Outdated
Show resolved
Hide resolved
2e6c1eb
to
8447901
Compare
@andresrc I've made some changes, and rebased against master to bring the parent pom forward (I didn't realise the branch was so far behind!) which I think might be better. I did keep getting EDIT: Never mind... |
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.
Thanks for addressing the feedback! Just a couple of additional comments.
|
||
@DataBoundSetter | ||
public void setStsTokenDuration(@NonNull Integer stsTokenDuration) { | ||
this.stsTokenDuration = stsTokenDuration.equals(DescriptorImpl.defaultStsTokenDuration) ? null : stsTokenDuration; |
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.
With this logic you can relax the @NonNull
condition, which is only used by static analysis and cannot be enforced by Jelly and write:
this.stsTokenDuration = stsTokenDuration == null || stsTokenDuration.equals(DescriptorImpl.defaultStsTokenDuration) ? null : stsTokenDuration;
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.
Thanks for the feedback. I've added the improvements for how the method handles null values now
@@ -176,10 +190,13 @@ public String getDisplayName() { | |||
return Messages.AWSCredentialsImpl_DisplayName(); | |||
} | |||
|
|||
public static final Integer defaultStsTokenDuration = STS_CREDENTIALS_DURATION_SECONDS; |
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.
Do we need another constant here? If it is to reference it from Jelly, ${it.STS_CREDENTIALS_DURATION_SECONDS}
should do the trick. Anyway by convention, constants should be all uppercase.
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.
I kept the original constant for backwards compatibility, as it was public. I tried incorporating your suggestion, but could not access STS_CREDENTIALS_DURATION_SECONDS
from Jelly using the it
syntax, so at the moment, I've kept it as it was, but changed the casing. Please let me know if this would be okay.
44bf265
to
68eda61
Compare
@andresrc I've made some changes. Awaiting feedback |
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.
Thanks for changes, just one more thing.
@@ -63,17 +65,18 @@ | |||
|
|||
private static final Logger LOGGER = Logger.getLogger(BaseAmazonWebServicesCredentials.class.getName()); | |||
|
|||
public static final int STS_CREDENTIALS_DURATION_SECONDS = 3600; | |||
public static final Integer STS_CREDENTIALS_DURATION_SECONDS = 3600; |
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.
No need to change the type here, as it will be boxed when needed. Besides, I'm not sure if it would be backwards compatible. Better to revert this change.
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.
Ah, I changed type because I was getting boxing then unboxing compilation errors with the new setStsTokenDuration
logic
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.
I’ll see what I can do to revert it when I get back to a computer
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.
Worst case:
this.stsTokenDuration = stsTokenDuration == null || stsTokenDuration.intValue() == STS_CREDENTIALS_DURATION_SECONDS ? null : stsTokenDuration;
…he existing constructor Recommendation based off of comment here: jenkinsci#43 (comment)
a3167db
to
eada550
Compare
@andresrc I kept hitting boxing and unboxing errors, so I've reverted to accessing the variable through |
eada550
to
e1e1d7d
Compare
e1e1d7d
to
63c1d2b
Compare
Based off of #43, but attempts to address the changes requested by @andresrc at #43 (review)