-
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
Add DeployTargetsByPlugin to Web API #5556
Conversation
Signed-off-by: khanhtc1202 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5556 +/- ##
==========================================
- Coverage 26.43% 26.42% -0.01%
==========================================
Files 465 465
Lines 49901 49913 +12
==========================================
Hits 13189 13189
- Misses 35657 35669 +12
Partials 1055 1055 ☔ View full report in Codecov by Sentry. |
@@ -360,7 +360,7 @@ func (a *API) UpdateApplication(ctx context.Context, req *apiservice.UpdateAppli | |||
return nil, status.Error(codes.InvalidArgument, "Requested piped does not belong to your project") | |||
} | |||
|
|||
if err := a.applicationStore.UpdateConfiguration(ctx, req.ApplicationId, req.PipedId, req.PlatformProvider, req.GitPath.ConfigFilename); err != nil { | |||
if err := a.applicationStore.UpdateConfiguration(ctx, req.ApplicationId, req.PipedId, req.PlatformProvider, req.GitPath.ConfigFilename, nil); err != nil { |
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.
Maybe add req.DeployTargetsByPlugin
as args?
if err := a.applicationStore.UpdateConfiguration(ctx, req.ApplicationId, req.PipedId, req.PlatformProvider, req.GitPath.ConfigFilename, nil); err != nil { | |
if err := a.applicationStore.UpdateConfiguration(ctx, req.ApplicationId, req.PipedId, req.PlatformProvider, req.GitPath.ConfigFilename, req.DeployTargetsByPlugin); err != nil { |
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 don't update api service.proto (API for pipectl and external tool), so there is no req.DeployTargetsByPlugin
, just pass nil here and not update the grpc for this API since this one is used by pipectl and outer tool (like https://github.com/pipe-cd/terraform-provider-pipecd), I will consider to update this later
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.
@khanhtc1202 I got it and OK to consider later. So it would be nice to add a comment about the context.
IMO, we need to update the API for pipectl and external tool because it will update as nil when adding the app and later update via pipectl and some other tool.
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.
@ffjlabo Ofcource, we will update pipectl commands related to applications later. I just do not want to update the API this time to confuse the outer tools dev because the pipedv1 plugin is not yet ready.
Also, I don't add comments here since it's not just this API but others like AddApplication that need to be revised later, too. The change here isn't caused by an API change but by an internal store interface change, so fewer comments are okay. We have to revise all the pipette commands later, so I think a comment is not needed.
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:
as title
Why we need it:
These API will be used by forms in Web UI to register and edit application
Which issue(s) this PR fixes:
Part of #5252
Does this PR introduce a user-facing change?: