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

Pass DeployTarget in SDK #5616

Merged
merged 8 commits into from
Mar 3, 2025
Merged

Pass DeployTarget in SDK #5616

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d5b9bdf
Pass DeployTarget in SDK
ffjlabo Mar 3, 2025
fa64fcd
Remove comment
ffjlabo Mar 3, 2025
8173d91
Refactor to do early continue
ffjlabo Mar 3, 2025
fbb133e
Fix error message
ffjlabo Mar 3, 2025
fc90b4c
Add the number of the deployTargets
ffjlabo Mar 3, 2025
03519ad
Add comment for caching
ffjlabo Mar 3, 2025
0e167c0
Allow no deploy targets in the plugin config considering the plugins …
ffjlabo Mar 3, 2025
1efc476
Refactor to return error early
ffjlabo Mar 3, 2025
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
28 changes: 27 additions & 1 deletion pkg/plugin/sdk/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,33 @@
stageID: request.GetInput().GetStage().GetId(),
logPersister: lp,
}
return executeStage(ctx, s.base, &s.config, nil, client, request, s.logger) // TODO: pass the deployTargets

dtNames, err := request.GetInput().GetDeployment().GetDeployTargets(s.commonFields.config.Name)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to get deploy targets for plugin %s: %v", s.commonFields.config.Name, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nits]
[nit]
I wanna clarify which one we mention,

  • the deployTraget was not found in a piped config
  • the deployTarget was not set in the request(deployment)
Suggested change
return nil, status.Errorf(codes.Internal, "failed to get deploy targets for plugin %s: %v", s.commonFields.config.Name, err)
return nil, status.Errorf(codes.Internal, "failed to get deploy targets from the deployment for plugin %s: %v", s.commonFields.config.Name, err)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed on fbb133e

}

Check warning on line 213 in pkg/plugin/sdk/deployment.go

View check run for this annotation

Codecov / codecov/patch

pkg/plugin/sdk/deployment.go#L209-L213 

Added lines #L209 - L213 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin which doesn't have deploy target configurations such as WAIT will return an error in this if statement, and it's not expected behavior.

ref;

pipecd/pkg/model/deployment.go

Lines 217 to 224 in 4afa3e6

func (d *Deployment) GetDeployTargets(pluginName string) ([]string, error) {
dps, ok := d.GetDeployTargetsByPlugin()[pluginName]
if !ok || len(dps.GetDeployTargets()) == 0 {
return nil, fmt.Errorf("deploy target not found for plugin %v", pluginName)
}
return dps.GetDeployTargets(), nil
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Warashi
Thanks.
I fixed the method logic to allow the case of no deploy target. We can decide whether it is an error or not in the plugin logic from now.
0e167c0


deployTargets := make([]*DeployTarget[DeployTargetConfig], 0, len(dtNames))
for _, name := range dtNames {
if dt := s.commonFields.config.FindDeployTarget(name); dt != nil {
var sdkDt DeployTargetConfig
if err := json.Unmarshal(dt.Config, &sdkDt); err != nil {
return nil, status.Errorf(codes.Internal, "failed to unmarshal deploy target config: %v", err)
}

Check warning on line 221 in pkg/plugin/sdk/deployment.go

View check run for this annotation

Codecov / codecov/patch

pkg/plugin/sdk/deployment.go#L215-L221 

Added lines #L215 - L221 were 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.

[IMO] If possible, it's better to cache DeployTargetConfigs in the server instead of repeating unmarshaling.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t-kikuc
Thanks, that sounds nice.
I added a TODO comment for it. I think it would be nice to focus on passing the deploy targets in this PR. Is it OK?
03519ad

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! TODO is best for this case!

We don't have to do that now.


deployTargets = append(deployTargets, &DeployTarget[DeployTargetConfig]{
Name: name,
Labels: dt.Labels,
Config: sdkDt,
})

Check warning on line 227 in pkg/plugin/sdk/deployment.go

View check run for this annotation

Codecov / codecov/patch

pkg/plugin/sdk/deployment.go#L223-L227 

Added lines #L223 - L227 were not covered by tests
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[IMO]
We can use the early-return pattern with the error containing the deployTarget name that is not found when dt == nil .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed on 8173d91

}

if len(dtNames) != len(deployTargets) {
return nil, status.Errorf(codes.Internal, "the number of deploy targets in the piped plugin config should be the same as the ones set on the deployment")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nits]
Let's add len(dtNames) and len(deployTargets) in the message to make it easier to troubleshoot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed on fc90b4c

}

Check warning on line 233 in pkg/plugin/sdk/deployment.go

View check run for this annotation

Codecov / codecov/patch

pkg/plugin/sdk/deployment.go#L231-L233 

Added lines #L231 - L233 were not covered by tests

return executeStage(ctx, s.base, &s.config, deployTargets, client, request, s.logger)

Check warning on line 235 in pkg/plugin/sdk/deployment.go

View check run for this annotation

Codecov / codecov/patch

pkg/plugin/sdk/deployment.go#L235 

Added line #L235 was not covered by tests
}

// StagePluginServiceServer is the gRPC server that handles requests from the piped.
Expand Down