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

Jump to bottom

resource: add MutateAndValidate endpoint #20311

Merged
merged 10 commits into from
Jan 25, 2024
Merged

resource: add MutateAndValidate endpoint #20311

merged 10 commits into from
Jan 25, 2024

Conversation

analogue
Copy link
Contributor

@analogue analogue commented Jan 22, 2024 •
edited
Loading

Description

See NET-6516 for context and acceptance criteria.

There is quite a bit of sharing between the Write endpoint and the new MutateAndValidate endpoint since Write is a superset. I've DRY'ed things up a bit but didn't go off the rails (especially with the tests) to draw a balance with maintainability. e.g. The tests share test structs and test cases but not test methods. Open to better ways to structure this with maintainability in mind.

Testing & Reproduction steps

  • DONE: Unit tests added
  • DONE: Add v2tenancy variations of tests but that shouldn't block reviewing the bulk of the existing PR.

Reviewers

  • Primary is @loshz for consul-core-platform
  • FYI @thisisnotashwin since you're planning to use this endpoint on the K8s side

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@analogue analogue added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-backport labels Jan 22, 2024
@analogue analogue marked this pull request as ready for review January 23, 2024 19:12
@analogue analogue requested a review from a team as a code owner January 23, 2024 19:12
@analogue analogue changed the title Spatel/mutate and validate resource: add MutateAndValidate endpoint Jan 23, 2024
loshz
loshz approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@loshz loshz left a comment

Choose a reason for hiding this comment

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

Overall LGTM and I trust your judgment on the implementation as you have the greatest knowledge in this domain.

RE your comment about test improvements: I like what you've done here 👍

agent/grpc-external/services/resource/mutate_and_validate.go
return false, status.Error(codes.InvalidArgument, err.Error())
}

// ACL check comes before tenancy existence checks to not leak tenancy "existence".
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

agent/grpc-external/services/resource/mutate_and_validate.go Show resolved Hide resolved
agent/grpc-external/services/resource/testing/testing_ce.go Outdated Show resolved Hide resolved
proto-public/pbresource/resource.proto Outdated Show resolved Hide resolved
@analogue analogue merged commit efdf804 into main Jan 25, 2024
91 checks passed
@analogue analogue deleted the spatel/mutate-and-validate branch January 25, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loshz loshz loshz approved these changes

@thisisnotashwin thisisnotashwin Awaiting requested review from thisisnotashwin

Assignees
No one assigned
Labels
pr/no-backport pr/no-changelog PR does not need a corresponding .changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@analogue @loshz