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

fix(deployments datasource): get by name #343

Merged
merged 4 commits into from
Dec 20, 2024
Merged

fix(deployments datasource): get by name #343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9f6f0f2
Deployment: Replace List with GetByName
mitchnielsen Dec 19, 2024
c2f155b
Validate either id or name and flow_name
mitchnielsen Dec 19, 2024
61b48e6
Specify flow_name in datasource test
mitchnielsen Dec 19, 2024
aa509ab
Generate Terraform Docs
github-actions[bot] Dec 19, 2024
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
7 changes: 5 additions & 2 deletions docs/data-sources/deployment.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@ data "prefect_deployment" "existing_by_id_string" {

# Get deployment by name using Terraform name reference.
data "prefect_deployment" "existing_by_id_string" {
name = prefect_deployment.my_existing_deployment.name
flow_name = prefect_flow.my_existing_flow.name
name = prefect_deployment.my_existing_deployment.name
}

# Get deployment by name string.
data "prefect_deployment" "existing_by_id_string" {
name = "my_existing_deployment"
name = "my_existing_deployment"
flow_name = "example_flow"
}
```

Expand All @@ -45,6 +47,7 @@ data "prefect_deployment" "existing_by_id_string" {
### Optional

- `account_id` (String) Account ID (UUID), defaults to the account set in the provider
- `flow_name` (String) Flow name associated with the deployment
- `id` (String) Deployment ID (UUID)
- `name` (String) Name of the deployment
- `workspace_id` (String) Workspace ID (UUID) to associate deployment to
Expand Down
6 changes: 4 additions & 2 deletions examples/data-sources/prefect_deployment/data-source.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ data "prefect_deployment" "existing_by_id_string" {

# Get deployment by name using Terraform name reference.
data "prefect_deployment" "existing_by_id_string" {
name = prefect_deployment.my_existing_deployment.name
flow_name = prefect_flow.my_existing_flow.name
name = prefect_deployment.my_existing_deployment.name
}

# Get deployment by name string.
data "prefect_deployment" "existing_by_id_string" {
name = "my_existing_deployment"
name = "my_existing_deployment"
flow_name = "example_flow"
}
2 changes: 1 addition & 1 deletion internal/api/deployments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
type DeploymentsClient interface {
Create(ctx context.Context, data DeploymentCreate) (*Deployment, error)
Get(ctx context.Context, deploymentID uuid.UUID) (*Deployment, error)
List(ctx context.Context, handleNames []string) ([]*Deployment, error)
GetByName(ctx context.Context, flowName, deploymentName string) (*Deployment, error)
Update(ctx context.Context, deploymentID uuid.UUID, data DeploymentUpdate) error
Delete(ctx context.Context, deploymentID uuid.UUID) error
}
Expand Down
23 changes: 12 additions & 11 deletions internal/client/deployments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,29 +60,30 @@ func (c *DeploymentsClient) Create(ctx context.Context, data api.DeploymentCreat
return &deployment, nil
}

// List returns a list of Deployments based on the provided list of names.
func (c *DeploymentsClient) List(ctx context.Context, _ []string) ([]*api.Deployment, error) {
// Get returns details for a Deployment by ID.
func (c *DeploymentsClient) Get(ctx context.Context, deploymentID uuid.UUID) (*api.Deployment, error) {
Comment on lines +63 to +64
Copy link
Contributor Author

@mitchnielsen mitchnielsen Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This diff shows things in kind of a weird way. I technically replaced List with GetByName.

So to clarify:

  • Get is unchanged
  • GetByName is added
  • List is removed

cfg := requestConfig{
method: http.MethodPost,
url: fmt.Sprintf("%s/filter", c.routePrefix),
method: http.MethodGet,
url: fmt.Sprintf("%s/%s", c.routePrefix, deploymentID.String()),
body: http.NoBody,
apiKey: c.apiKey,
successCodes: successCodesStatusOK,
}

var deployments []*api.Deployment
if err := requestWithDecodeResponse(ctx, c.hc, cfg, &deployments); err != nil {
return nil, fmt.Errorf("failed to list deployments: %w", err)
var deployment api.Deployment
if err := requestWithDecodeResponse(ctx, c.hc, cfg, &deployment); err != nil {
return nil, fmt.Errorf("failed to get deployment: %w", err)
}

return deployments, nil
return &deployment, nil
}

// Get returns details for a Deployment by ID.
func (c *DeploymentsClient) Get(ctx context.Context, deploymentID uuid.UUID) (*api.Deployment, error) {
// GetByName returns details for a Deployment by name.
func (c *DeploymentsClient) GetByName(ctx context.Context, flowName, deploymentName string) (*api.Deployment, error) {
url := fmt.Sprintf("%s/name/%s/%s", c.routePrefix, flowName, deploymentName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It surprised me that the API requires the flow_name to get a deployment by name. I started by adding that field as an optional attribute on the datasource specifically, and then validate that either id or name and flow_name are set.

Alternatives considered so far:

  • Going back to List/Filter, and passing the filter payload: the filter payload is pretty complex, and I'm not keen to model the payload and response in Terraform tbh
  • Going back to List/Filter, and getting all Deployments then finding the one with a matching name: this doesn't quite feel right given the potential size of the response, and I have this feeling that flow_name is required for a reason - maybe Deployments can have the same name if they're associated with different flows?
  • Automatically getting the flow_name for the user based on the flow_id: we already have the flow_id set on the Deployment resource, so this would be easier for users in some cases (such as when they don't define the flow in Terraform and therefore don't have access to prefect_flow.example.name). This is a fairly simple API call, and is probably my preference out of these options so far, but still doesn't quite feel optimal.

Open to any opinions here

Copy link
Contributor

@parkedwards parkedwards Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, the option we choose will depend on what interface we're choosing to expose to the user, which is coupled to what we think they're going to use the datasource for:

  • search a single deployment by name => return a single Deployment. since deployment names aren't unique + need an extra piece of info to triangulate (the flow itself), we could either implicitly look up the flow as you suggested OR expect the user to include that flow identifier in the datasource schema (so they'd put in a deployment name + flow name) and pass it into the filter call (see next bullet about the Deployment filter object)
  • search all deployments with a name => return a list of Deployments. use the /filter endpoint, and model out the payload. The Deployment filter schema is pretty big and includes a bunch of searchable options (work pool, flow name, etc.) so I'd advocate for investing in defining this. We don't have to model the entire filter payload to start, it could look something like this
type DeploymentFilter struct {
	Deployments struct {
		Name struct {
			Any []string `json:"any_"`
		} `json:"name"`
	} `json:"deployments"`
}

# usage:
filter := DeploymentFilter{}
filter.Deployments.Name.Any = []string{"deployment-1", "deployment-2"}

I don't think the "find all deployments and do the filtering at runtime" is better than either of those options, just given that we have a filtering endpoint that pretty much does this if we're able to represent the filtering payload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great points - I could see that advanced datasource logic being really helpful. I created #344 to track that. I think for now it makes sense to proceed with the current PR since it addresses the bug based on how we currently say this datasource should work. That's just my 2 cents though, happy to pivot

cfg := requestConfig{
method: http.MethodGet,
url: fmt.Sprintf("%s/%s", c.routePrefix, deploymentID.String()),
url: url,
body: http.NoBody,
apiKey: c.apiKey,
successCodes: successCodesStatusOK,
Expand Down
42 changes: 25 additions & 17 deletions internal/provider/datasources/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ type deploymentDataSource struct {
type DeploymentDataSourceModel struct {
// The model requires the same fields, so reuse the fields defined in the resource model.
resources.DeploymentResourceModel

// The following fields are specific to the Deployment datasource.
FlowName types.String `tfsdk:"flow_name"`
}

// NewDeploymentDataSource is a helper function to simplify the provider implementation.
Expand Down Expand Up @@ -94,6 +97,13 @@ The Deployment ID takes precedence over deployment name.
CustomType: customtypes.UUIDType{},
Description: "Flow ID (UUID) to associate deployment to",
},
// flow_name is specific to the datasource because it's used in the API endpoint
// to find a deployment by name.
"flow_name": schema.StringAttribute{
Computed: true,
Optional: true,
Description: "Flow name associated with the deployment",
},
"paused": schema.BoolAttribute{
Computed: true,
Description: "Whether or not the deployment is paused.",
Expand Down Expand Up @@ -243,7 +253,10 @@ func (d *deploymentDataSource) Read(ctx context.Context, req datasource.ReadRequ
// If both are set, we prefer the ID
var deployment *api.Deployment
var operation string
if !model.ID.IsNull() {
var getErr error

switch {
case !model.ID.IsNull():
var deploymentID uuid.UUID
deploymentID, err = uuid.Parse(model.ID.ValueString())
if err != nil {
Expand All @@ -257,25 +270,20 @@ func (d *deploymentDataSource) Read(ctx context.Context, req datasource.ReadRequ
}

operation = "get"
deployment, err = client.Get(ctx, deploymentID)
} else if !model.Name.IsNull() {
var deployments []*api.Deployment
operation = "list"
deployments, err = client.List(ctx, []string{model.Name.ValueString()})

if len(deployments) != 1 {
resp.Diagnostics.AddError(
"Could not find Deployment",
fmt.Sprintf("Could not find Deployment with name %s", model.Name.ValueString()),
)
deployment, getErr = client.Get(ctx, deploymentID)
case !model.FlowName.IsNull() && !model.Name.IsNull():
operation = "get by name"
deployment, getErr = client.GetByName(ctx, model.FlowName.ValueString(), model.Name.ValueString())
default:
resp.Diagnostics.AddError(
"Either id, or name and flow_name are unset",
"Please configure either id, or name and flow_name.",
)

return
}

deployment = deployments[0]
return
}

if err != nil {
if getErr != nil {
resp.Diagnostics.Append(helpers.ResourceClientErrorDiagnostic("Deployment", operation, err))

return
Expand Down
1 change: 1 addition & 0 deletions internal/provider/datasources/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ data "prefect_deployment" "test_by_id" {

data "prefect_deployment" "test_by_name" {
name = prefect_deployment.test.name
flow_name = prefect_flow.test.name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect this was only passing before because there was only one deployment in the test workspace, meaning the existing logic actually worked. This obviously breaks in the real world when there is more than one deployment (of any name).


account_id = "%s"
workspace_id = prefect_workspace.test.id
Expand Down