-
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
[WAIT] Use metadataStore in the wait stage plugin #5520
Conversation
Signed-off-by: t-kikuc <[email protected]>
Signed-off-by: t-kikuc <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5520 +/- ##
==========================================
- Coverage 26.43% 26.43% -0.01%
==========================================
Files 464 464
Lines 49786 49793 +7
==========================================
Hits 13163 13163
- Misses 35569 35576 +7
Partials 1054 1054 ☔ View full report in Codecov by Sentry. |
func (s *deploymentServiceServer) retrieveStartTime(ctx context.Context, deploymentID, stageID string) (t time.Time) { | ||
sec, err := s.metadataStore.GetStageMetadata(ctx, &pipedservice.GetStageMetadataRequest{ | ||
DeploymentId: deploymentID, | ||
StageId: stageID, | ||
Key: startTimeKey, | ||
}) | ||
if err != nil { | ||
s.logger.Error(fmt.Sprintf("failed to get stage metadata %s", startTimeKey), zap.Error(err)) | ||
return | ||
} | ||
ut, err := strconv.ParseInt(sec.Value, 10, 64) | ||
if err != nil { | ||
s.logger.Error(fmt.Sprintf("failed to parse stage metadata %s", startTimeKey), zap.Error(err)) | ||
return |
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.
[IMO]
I don't like naked returns, and named result parameters that don't clarify the usage of their return values.
ref; https://go.dev/wiki/CodeReviewComments#named-result-parameters
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.
Signed-off-by: t-kikuc <[email protected]>
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
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.
👍
What this PR does:
as title
Why we need it:
Which issue(s) this PR fixes:
Part of #5367
Does this PR introduce a user-facing change?: