fix(deployments): refactor pull step payload structure #322
Conversation
Pull steps have a specific format in the API payload that isn't very clearly defined by the API docs, since the payload value is an empty list. Looking more at the source code and documentation, I adjusted the payload sent to the API to match the expected format. The format of the Terraform resource stays the same. Overall changes: * Introduced PullStepCommon to encapsulate shared fields across pull step types. * Created specific types for PullStepGitClone, PullStepSetWorkingDirectory, and PullStepPullFrom to improve clarity and maintainability. This is where we can define the correct JSON field name for the payload. * Updated the mapping functions to accommodate the new structure, ensuring proper handling of pull steps in both API and Terraform contexts. Closes #321
mitchnielsen
requested a deployment
to
Acceptance Tests
GitHub Actions
Waiting
mitchnielsen
requested a deployment
to
Acceptance Tests
GitHub Actions
Waiting
mitchnielsen
requested a deployment
to
Acceptance Tests
GitHub Actions
Waiting
mitchnielsen
force-pushed
the
fix-pull-steps
branch
from
034d745 to
59da64d
Compare
mitchnielsen
temporarily deployed
to
Acceptance Tests
GitHub Actions
Inactive
| var apiPullStep api.PullStep | ||
| switch tfPullStep.Type.ValueString() { | ||
| case "git_clone": | ||
| apiPullStep.PullStepGitClone = &api.PullStepGitClone{ | ||
| PullStepCommon: pullStepCommon, | ||
| Repository: tfPullStep.Repository.ValueStringPointer(), | ||
| Branch: tfPullStep.Branch.ValueStringPointer(), | ||
| AccessToken: tfPullStep.AccessToken.ValueStringPointer(), | ||
| IncludeSubmodules: tfPullStep.IncludeSubmodules.ValueBoolPointer(), | ||
| } |
There was a problem hiding this comment.
This is obviously lengthier than the logic we defined before, but because of the payload expected by the API, I have to put configuration under certain fields in the parent struct so that I can give that parent struct the correct JSON struct tag like prefect.deployments.steps.git_clone.
| @@ -924,15 +1007,29 @@ func (r *DeploymentResource) ImportState(ctx context.Context, req resource.Impor | |||
| // be more concise when defining the conflicting attributes. Defining them in | |||
| // ConfigValidators instead would be much more verbose, and disconnected from | |||
| // the source of truth. | |||
| func validatorsForConflictingAttributes(attributes []string) []validator.String { | |||
| func pathExpressionsForAttributes(attributes []string) []path.Expression { | |||
There was a problem hiding this comment.
Refactored this slightly - it was set up to always return string validators, but now that we have IncludeSubmodules, we need bool validators as well. So I abstracted this one to return only path expressions, then two small helpers below that wrap this function and return the desired validator type.
| @@ -101,33 +101,80 @@ resource "prefect_deployment" "{{.DeploymentName}}" { | |||
| work_queue_name = "{{.WorkQueueName}}" | |||
| parameter_openapi_schema = jsonencode({{.ParameterOpenAPISchema}}) | |||
| pull_steps = [ | |||
| {{range .PullSteps}} | |||
| {{range .PullSteps}} | |||
There was a problem hiding this comment.
The changes below are a bit hard to read, and repetitive. I tried importing masterminds/sprig to give us coalesce and some other helpers, but it started escaping quotations and I couldn't find any reference as to why that was happening. I ditched that to stick with the standard library's template and use or, which seemed to work fine at giving me a context based on which field wasn't empty - but then I was getting errors that it couldn't access nested fields when they were undefined.
This may actually be doable, but to get this PR rolling I just added a little duplication for the common fields and moved on.
| @@ -380,7 +436,7 @@ func testAccCheckDeploymentValues(fetchedDeployment *api.Deployment, expectedVal | |||
| } | |||
|
|
|||
| if !reflect.DeepEqual(fetchedDeployment.PullSteps, expectedValues.pullSteps) { | |||
| return fmt.Errorf("Expected pull steps to be %v, got %v", expectedValues.pullSteps, fetchedDeployment.PullSteps) | |||
| return fmt.Errorf("Expected pull steps to be: \n%v\n got \n%v", expectedValues.pullSteps, fetchedDeployment.PullSteps) | |||
There was a problem hiding this comment.
Non-critical change, but made it much easier to read what the difference was between the two objects.
mitchnielsen
temporarily deployed
to
Acceptance Tests
GitHub Actions
Inactive
|
True, seems easier than Automations though :p |
mitchnielsen
changed the title
Summary
Pull steps have a specific format in the API payload
that isn't very clearly defined by the API docs, since
the payload value is an empty list.
Looking more at the source code and documentation,
I adjusted the payload sent to the API to match the
expected format.
The format of the Terraform resource stays the same.
For reference, the incorrect format we had was:
[ { "type": "git_clone", "branch": "main", "repository": "https://my-git-repo.git", "access_token": "fake-token" } ]The correct format is:
[ { "prefect.deployments.steps.git_clone": { "branch": "main", "repository": "https://my-git-repo.git", "access_token": "fake-token" } } ]Overall changes:
IncludeSubmodulesfield, which I missed on the first passCloses #321
Testing
Acceptance tests
Check out the acceptance tests added in the PR.
Manual tests
Also manually tested this to validate #321 is addressed:
Deployed the configuration from Flow run crashed with ValueError: Step has unexpected additional keys: branch, repository, access_token #321.
Confirmed the Deployment shows the correct Pull Step configuration:
Clicked "Quick run" and confirmed that the flow ran successfully after cloning the repository per the pull steps: