-
Notifications
You must be signed in to change notification settings - Fork 157
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
Refactor the plugin SDK to set common fields and configs in a single method #5623
Conversation
…method Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5623 +/- ##
==========================================
- Coverage 26.54% 26.54% -0.01%
==========================================
Files 477 477
Lines 50663 50666 +3
==========================================
Hits 13450 13450
- Misses 36150 36153 +3
Partials 1063 1063 ☔ View full report in Codecov by Sentry. |
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.
LGTM
…y-target-prepare Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
@t-kikuc I merged origin/master. Please re-approve 🙏🏻 |
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!
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.
LGTM
What this PR does:
setCommonFields
andsetConfig
assetFields
DeployTargets
insetFields
method and store it as aDeploymentPluginServiceServer.deployTargets
Why we need it:
TODO: cache the unmarshaled config to avoid unmarshaling it multiple times.
Which issue(s) this PR fixes:
Part of #5530
Does this PR introduce a user-facing change?: