Skip to content
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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Add DeployTargetsByPlugin to Web API #5556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/app/server/grpcapi/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
Enable(ctx context.Context, id string) error
Disable(ctx context.Context, id string) error
UpdateConfigFilename(ctx context.Context, id, filename string) error
UpdateConfiguration(ctx context.Context, id, pipedID, platformProvider, configFilename string) error
UpdateConfiguration(ctx context.Context, id, pipedID, platformProvider, configFilename string, deployTargetsByPlugin map[string]*model.DeployTargets) error
UpdateDeployTargets(ctx context.Context, id string, dp map[string]*model.DeployTargets) error
}

Expand Down Expand Up @@ -360,7 +360,7 @@
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 {

Check warning on line 363 in pkg/app/server/grpcapi/api.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/server/grpcapi/api.go#L363 

Added line #L363 was not covered by tests
Copy link
Member

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?

Suggested change
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 {

Copy link
Member Author

@khanhtc1202 khanhtc1202 Feb 7, 2025 •
edited
Loading

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

Copy link
Member

@ffjlabo ffjlabo Feb 10, 2025 •
edited
Loading

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.

Copy link
Member Author

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.

return nil, gRPCStoreError(err, fmt.Sprintf("failed to update application %s", req.ApplicationId))
}

Expand Down
25 changes: 13 additions & 12 deletions pkg/app/server/grpcapi/web_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
Get(ctx context.Context, id string) (*model.Application, error)
List(ctx context.Context, opts datastore.ListOptions) ([]*model.Application, string, error)
Delete(ctx context.Context, id string) error
UpdateConfiguration(ctx context.Context, id, pipedID, platformProvider, configFilename string) error
UpdateConfiguration(ctx context.Context, id, pipedID, platformProvider, configFilename string, deployTargetsByPlugin map[string]*model.DeployTargets) error
Enable(ctx context.Context, id string) error
Disable(ctx context.Context, id string) error
}
Expand Down Expand Up @@ -512,16 +512,17 @@
}

app := model.Application{
Id: uuid.New().String(),
Name: req.Name,
PipedId: req.PipedId,
ProjectId: claims.Role.ProjectId,
GitPath: gitpath,
Kind: req.Kind,
PlatformProvider: req.PlatformProvider,
CloudProvider: req.PlatformProvider,
Description: req.Description,
Labels: req.Labels,
Id: uuid.New().String(),
Name: req.Name,
PipedId: req.PipedId,
ProjectId: claims.Role.ProjectId,
GitPath: gitpath,
Kind: req.Kind,
PlatformProvider: req.PlatformProvider,
CloudProvider: req.PlatformProvider,
DeployTargetsByPlugin: req.DeployTargetsByPlugin,
Description: req.Description,
Labels: req.Labels,

Check warning on line 525 in pkg/app/server/grpcapi/web_api.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/server/grpcapi/web_api.go#L515-L525 

Added lines #L515 - L525 were not covered by tests
}
if err = a.applicationStore.Add(ctx, &app); err != nil {
return nil, gRPCStoreError(err, fmt.Sprintf("add application %s", app.Id))
Expand All @@ -548,7 +549,7 @@
return nil, status.Error(codes.PermissionDenied, "Requested piped does not belong to your project")
}

if err := a.applicationStore.UpdateConfiguration(ctx, req.ApplicationId, req.PipedId, req.PlatformProvider, req.ConfigFilename); err != nil {
if err := a.applicationStore.UpdateConfiguration(ctx, req.ApplicationId, req.PipedId, req.PlatformProvider, req.ConfigFilename, req.DeployTargetsByPlugin); err != nil {

Check warning on line 552 in pkg/app/server/grpcapi/web_api.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/server/grpcapi/web_api.go#L552 

Added line #L552 was not covered by tests
return nil, gRPCStoreError(err, fmt.Sprintf("failed to update application %s", req.ApplicationId))
}

Expand Down
2,217 changes: 1,135 additions & 1,082 deletions pkg/app/server/service/webservice/service.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

114 changes: 114 additions & 0 deletions pkg/app/server/service/webservice/service.pb.validate.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions pkg/app/server/service/webservice/service.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ message AddApplicationRequest {
model.ApplicationGitPath git_path = 4 [(validate.rules).message.required = true];
model.ApplicationKind kind = 5 [(validate.rules).enum.defined_only = true];
string platform_provider = 9;
map<string, model.DeployTargets> deploy_targets_by_plugin = 10 [(validate.rules).map.min_pairs = 1];
string description = 7;
map<string, string> labels = 8;
}
Expand All @@ -379,6 +380,7 @@ message UpdateApplicationRequest {
string piped_id = 4 [(validate.rules).string.min_len = 1];
model.ApplicationKind kind = 6 [(validate.rules).enum.defined_only = true];
string platform_provider = 9;
map<string, model.DeployTargets> deploy_targets_by_plugin = 10 [(validate.rules).map.min_pairs = 1];
string config_filename = 8;
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/datastore/applicationstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@
UpdateConfigFilename(ctx context.Context, id, configFilename string) error
UpdateDeployingStatus(ctx context.Context, id string, deploying bool) error
UpdateBasicInfo(ctx context.Context, id, name, description string, labels map[string]string) error
UpdateConfiguration(ctx context.Context, id, pipedID, platformProvider, configFilename string) error
UpdateConfiguration(ctx context.Context, id, pipedID, platformProvider, configFilename string, deployTargetsByPlugin map[string]*model.DeployTargets) error
UpdatePlatformProvider(ctx context.Context, id string, provider string) error
UpdateDeployTargets(ctx context.Context, id string, dp map[string]*model.DeployTargets) error
}
Expand Down Expand Up @@ -362,11 +362,12 @@
})
}

func (s *applicationStore) UpdateConfiguration(ctx context.Context, id, pipedID, platformProvider, configFilename string) error {
func (s *applicationStore) UpdateConfiguration(ctx context.Context, id, pipedID, platformProvider, configFilename string, deployTargetsByPlugin map[string]*model.DeployTargets) error {

Check warning on line 365 in pkg/datastore/applicationstore.go

View check run for this annotation

Codecov / codecov/patch

pkg/datastore/applicationstore.go#L365 

Added line #L365 was not covered by tests
return s.update(ctx, id, func(app *model.Application) error {
app.PipedId = pipedID
app.PlatformProvider = platformProvider
app.CloudProvider = platformProvider
app.DeployTargetsByPlugin = deployTargetsByPlugin

Check warning on line 370 in pkg/datastore/applicationstore.go

View check run for this annotation

Codecov / codecov/patch

pkg/datastore/applicationstore.go#L370 

Added line #L370 was not covered by tests
app.GitPath.ConfigFilename = configFilename
return nil
})
Expand Down
8 changes: 4 additions & 4 deletions pkg/datastore/datastoretest/datastore.mock.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions web/api_client/service_pb.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,9 @@ export class AddApplicationRequest extends jspb.Message {
getPlatformProvider(): string;
setPlatformProvider(value: string): AddApplicationRequest;

getDeployTargetsByPluginMap(): jspb.Map<string, pkg_model_deployment_pb.DeployTargets>;
clearDeployTargetsByPluginMap(): AddApplicationRequest;

getDescription(): string;
setDescription(value: string): AddApplicationRequest;

Expand All @@ -520,6 +523,7 @@ export namespace AddApplicationRequest {
gitPath?: pkg_model_common_pb.ApplicationGitPath.AsObject,
kind: pkg_model_common_pb.ApplicationKind,
platformProvider: string,
deployTargetsByPluginMap: Array<[string, pkg_model_deployment_pb.DeployTargets.AsObject]>,
description: string,
labelsMap: Array<[string, string]>,
}
Expand Down Expand Up @@ -559,6 +563,9 @@ export class UpdateApplicationRequest extends jspb.Message {
getPlatformProvider(): string;
setPlatformProvider(value: string): UpdateApplicationRequest;

getDeployTargetsByPluginMap(): jspb.Map<string, pkg_model_deployment_pb.DeployTargets>;
clearDeployTargetsByPluginMap(): UpdateApplicationRequest;

getConfigFilename(): string;
setConfigFilename(value: string): UpdateApplicationRequest;

Expand All @@ -577,6 +584,7 @@ export namespace UpdateApplicationRequest {
pipedId: string,
kind: pkg_model_common_pb.ApplicationKind,
platformProvider: string,
deployTargetsByPluginMap: Array<[string, pkg_model_deployment_pb.DeployTargets.AsObject]>,
configFilename: string,
}
}
Expand Down
Loading