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

refactor: make Kargo Roles proper runtime objects #1917

Merged
merged 9 commits into from
May 1, 2024

Conversation

krancour
Copy link
Member

@krancour krancour commented May 1, 2024

Follow up to #1906 and related to #1916

Kargo Roles are the one place in all of our APIs where protobuf messages might be shown directly to a user -- which is how I became aware of #1916.

While there may be some things that can be done about #1916, I recognize that protobuf field names should only matter over the wire and are, essentially, an implementation detail that really never should bleed through to the user. i.e. I don't want to get hung up on fixing #1916 to force json struct tags to look a certain go-ish way.

This sent me down the rabbit hole of trying something new, that turns out to solve a lot of problems at once...

  1. I created a new rbac.kargo.akuity.io/v1alpha1 API wherein Roles are proper k8s runtime objects (although we skip the CRD generation for these, because it's not needed)

  2. This drastically cleans up the RoleDatabase interface, because it now deals with rbacapi.Role objects instead of dealing directly with protobuf messages -- something it probably shouldn't have done in the first place.

  3. Protobuf messages like GetRoleRequest now wrap Role messages that are generated from the Role runtime object -- i.e. we now handle this exactly as we have always handled GetStageRequest, GetPromotionRequest, etc., which have also always wrapped messages generated from runtime objects. Consistency ftw.

  4. Any JSON or YAML the CLI shows to users now is more conventionally k8s-looking (fields names are lowerCamelCase instead of snake_case).

  5. Last, but certainly not least, Roles being proper runtime objects eliminates all the slight of hand that was previously required to make

There are also some tiny bug fixes to revoke logic in here.

krancour added 8 commits May 1, 2024 03:00
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Copy link

netlify bot commented May 1, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit cde3a98
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/6631f5e0a2eac9000801b887
😎 Deploy Preview https://deploy-preview-1917.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented May 1, 2024

Codecov Report

Attention: Patch coverage is 35.50725% with 89 lines in your changes are missing coverage. Please review.

Project coverage is 45.58%. Comparing base (c6d0a6d) to head (cde3a98).

Files Patch % Lines
internal/api/rbac/roles.go 75.38% 12 Missing and 4 partials ⚠️
internal/cli/cmd/get/roles.go 0.00% 15 Missing ⚠️
internal/api/list_roles_v1alpha1.go 0.00% 10 Missing ⚠️
internal/cli/cmd/create/role.go 0.00% 9 Missing ⚠️
api/rbac/v1alpha1/groupversion_info.go 0.00% 8 Missing ⚠️
internal/api/get_role_v1alpha1.go 0.00% 8 Missing ⚠️
internal/cli/cmd/delete/role.go 0.00% 5 Missing ⚠️
internal/cli/cmd/grant/grant.go 0.00% 3 Missing ⚠️
internal/cli/cmd/revoke/revoke.go 0.00% 3 Missing ⚠️
internal/kubeclient/indexer.go 0.00% 3 Missing ⚠️
... and 6 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1917      +/-   ##
==========================================
+ Coverage   45.52%   45.58%   +0.06%     
==========================================
  Files         235      235              
  Lines       15823    15793      -30     
==========================================
- Hits         7203     7199       -4     
+ Misses       8267     8240      -27     
- Partials      353      354       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

👏!

@krancour krancour added this pull request to the merge queue May 1, 2024
Merged via the queue into akuity:main with commit af4feb5 May 1, 2024
15 of 16 checks passed
@krancour krancour deleted the krancour/role-mgmt-fixes branch May 1, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants