fix(deployments): align omitempty usage #357
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Aligns the omitempty usage with the expectations from the API. Required fields should not set omitempty. Optional fields should set omitempty, when we don't want to send empty values. We don't need omitempty on the object struct, just for structs used in API calls that get (un)marshalled. Closes https://linear.app/prefect/issue/PLA-919/manifest-path-causes-deployment-update-to-fail-prefect-server Closes #356
mitchnielsen
temporarily deployed
to
Acceptance Tests
GitHub Actions
Inactive
mitchnielsen
commented
Jan 15, 2025
| @@ -74,7 +74,7 @@ type DeploymentUpdate struct { | |||
| EnforceParameterSchema bool `json:"enforce_parameter_schema,omitempty"` | |||
| Entrypoint string `json:"entrypoint,omitempty"` | |||
| JobVariables map[string]interface{} `json:"job_variables,omitempty"` | |||
| ManifestPath string `json:"manifest_path"` | |||
| ManifestPath string `json:"manifest_path,omitempty"` | |||
There was a problem hiding this comment.
This is the fix for #356 specifically.
| @@ -50,10 +50,10 @@ type DeploymentCreate struct { | |||
| Description string `json:"description,omitempty"` | |||
| EnforceParameterSchema bool `json:"enforce_parameter_schema,omitempty"` | |||
| Entrypoint string `json:"entrypoint,omitempty"` | |||
| FlowID uuid.UUID `json:"flow_id"` | |||
| FlowID uuid.UUID `json:"flow_id"` // required | |||
There was a problem hiding this comment.
There's no required option (that I'm aware of), so I added // required here to signify that we're intentionally not specifying omitempty. My thought process was this may help us in the future to determine whether the field was left off intentionally or not - open to opinions though
| @@ -22,25 +22,25 @@ type Deployment struct { | |||
| WorkspaceID uuid.UUID `json:"workspace_id"` | |||
|
|
|||
| ConcurrencyLimit *int64 `json:"concurrency_limit"` | |||
| ConcurrencyOptions *ConcurrencyOptions `json:"concurrency_options,omitempty"` | |||
| Description string `json:"description,omitempty"` | |||
| ConcurrencyOptions *ConcurrencyOptions `json:"concurrency_options"` | |||
There was a problem hiding this comment.
AFAIK, there's no need to specify omitempty on the Deployment object itself because it's not used in any API requests and is therefore never (un)marshalled by the json package.
jamiezieziula
approved these changes
Jan 15, 2025
parkedwards
approved these changes
Jan 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Aligns the omitempty usage with the expectations from the API.
Required fields should not set omitempty.
Optional fields should set omitempty, when we don't want to send empty values.
We don't need omitempty on the object struct, just for structs used in API calls that get (un)marshalled.
Closes https://linear.app/prefect/issue/PLA-919/manifest-path-causes-deployment-update-to-fail-prefect-server
Closes #356