-
Notifications
You must be signed in to change notification settings - Fork 53
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
Fixes #444 - Add duration parameter to WithAwsPlugin.withRole() #445
Conversation
Codecov ReportBase: 82.52% // Head: 82.35% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #445 +/- ##
==========================================
- Coverage 82.52% 82.35% -0.18%
==========================================
Files 54 54
Lines 1511 1519 +8
Branches 322 323 +1
==========================================
+ Hits 1247 1251 +4
- Misses 115 117 +2
- Partials 149 151 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
This is working in at least one pipeline, now! |
|
||
``` | ||
WithAwsPlugin.withRole(null, 43200).init() | ||
``` |
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 fix looks good overall, but one minor tweak.
This feature is an example of trying to configure a plugin that has multiple different parameters, which can be set independently of one another.
Rather than trying to overload the .withRole()
method to optionally accept a role/role+duration/duration, can we simply add a .withDuration()
method? That way, if you want to set duration, you can do so explicitly. If you want to set the role, you can do so explicitly. If you want to set them both, then you chain and call them both explicitly.
By using the suggestion above, we can avoid the awkardness and lack-of-context in your example above: WithAwsPlugin.withRole(null, 43200)
. "What is 43200? What does that have to do with a null role?" vs WithAwsPlugin.withDuration(43200)
which gives context to 43200. "43200 is a duration".
This is a convention that's used heavily across all the plugins, so I'd like to keep that consistent. https://en.wikipedia.org/wiki/Fluent_interface
Rebased on master, and finally getting around to making the changes requested in the previous comment. |
@kmanning I've (finally) updated this PR as per the discussion above. Note that while the unit tests pass, I have not tested this in an actual pipeline yet, as I don't have one appropriate for that. @bgshacklett Is there any chance that, when you have time, you could update your pipeline with these changes? |
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 am continuing where @bgshacklett left off. This change has been tested internally and is ready to be merged and a release cut. Thank You. |
Work in progress PR to fix #444 by adding an optional
duration
parameter to WithAwsPlugin.withRole().Merge Checklist
./gradlew check --info
?