-
Notifications
You must be signed in to change notification settings - Fork 186
expose stage info created by buildpipeline phase to executestage phase. add null check for getDeploymentPluginMetadata #5910
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
Conversation
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.
Please describe your design on the issue in advance and submit the next draft PR because I cannot imagine what you are going to do.
pkg/plugin/sdk/client.go
Outdated
func (c *Client) GetDeploymentPluginMetadata(ctx context.Context, key string) (string, bool, error) { | ||
resp, err := c.base.GetDeploymentPluginMetadata(ctx, &pipedservice.GetDeploymentPluginMetadataRequest{ | ||
DeploymentId: c.deploymentID, | ||
PluginName: c.pluginName, | ||
Key: key, | ||
}) | ||
return resp.Value, err | ||
if err != nil { | ||
return "", false, err | ||
} | ||
return resp.Value, resp.Found, err | ||
} |
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.
Please split the PR since this does not seem to be directly related to SCRIPT_RUN.
And please make consistent with other GetXxxMetadata() funcs.
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.
currently this getxxxMetadata function's error checking is missing, which would cause nil pointer deref panic when the grpc call response with error. About the return signature of this format, it currently only extracts value property and not the found one, which would make it hard to differentiate, for example when the metadata is stored with empty value. Do you think the signature should stay as before, or as updated? @t-kikuc
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5910 +/- ##
===========================================
+ Coverage 28.23% 54.90% +26.67%
===========================================
Files 512 90 -422
Lines 55326 8771 -46555
===========================================
- Hits 15620 4816 -10804
+ Misses 38461 3625 -34836
+ Partials 1245 330 -915
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2db8bee
to
1d072f8
Compare
Signed-off-by: hiep-tk <[email protected]>
Signed-off-by: hiep-tk <[email protected]>
@hiep-tk |
What this PR does: as title
Why we need it: currently the script_run stages need more context to publish their stage already run status to the script_run_rollback so multiple rollbacks can be differentiated and selectively run
Which issue(s) this PR fixes: Part of #5901
Fixes #
Does this PR introduce a user-facing change?: