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.

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 further patch mechanisms #229

Merged
merged 4 commits into from
Feb 25, 2023

Conversation

kennethito
Copy link
Contributor

@kennethito kennethito commented Feb 17, 2023

Adds Operation.header_params which are used with K8s.Client.patch and K8s.Client.apply, allowing for alternate patch types.

Resolves #227

@kennethito kennethito marked this pull request as ready for review February 17, 2023 21:58
lib/k8s/operation.ex Outdated Show resolved Hide resolved
Copy link
Collaborator

@mruoss mruoss left a comment

Choose a reason for hiding this comment

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

Yes this looks like what I had in mind. Some comments. Let me know what you think.

lib/k8s/client.ex Outdated Show resolved Hide resolved
lib/k8s/client.ex Show resolved Hide resolved
lib/k8s/operation.ex Show resolved Hide resolved
lib/k8s/operation.ex Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
defstruct method: nil,
verb: nil,
api_version: nil,
name: nil,
data: nil,
conn: nil,
path_params: [],
query_params: []
query_params: [],
header_params: []
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we define a default here so we can safely remove the code in base.ex? Just a thought. Would have to run tests.

Suggested change
header_params: []
header_params: ["Content-Type": "application/json"]

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 did add this, and mix test is succeeding. I do think there is a case, where if an application was creating the Operation struct themselves, without using build, where this would be a breaking change if the Operation was a patch or apply, since it was the Client.Runner.Base that was setting these headers before. That seems like it would be really rare though?

@kennethito kennethito changed the title add operation header_params Add further patch mechanisms Feb 25, 2023
@kennethito
Copy link
Contributor Author

Oops, didn't realize pushing a commit would clear the workflow results. Basically everything passed except the mix format --check-formatting step.

@mruoss
Copy link
Collaborator

mruoss commented Feb 25, 2023

Credo not happy about complexity. But that's okay, I can take it from here. Will merge and fix Credo. Thanks for your work, @kennethito!

@mruoss mruoss merged commit e5e8bb3 into coryodaniel:develop Feb 25, 2023
@kennethito kennethito deleted the operation-header-params branch February 25, 2023 19:55
@mruoss
Copy link
Collaborator

mruoss commented Feb 25, 2023

Credo fixed in 7d2c744 (and a tiny refactoring)

@kennethito
Copy link
Contributor Author

Credo fixed in 7d2c744 (and a tiny refactoring)

Niceee, I like it.

@mruoss
Copy link
Collaborator

mruoss commented Feb 25, 2023

Integration tests would have been nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Patch strategy should be configurable
2 participants