Clean up access list protos, add in conversion functions tests. #28787
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
smallinsky
reviewed
Jul 7, 2023
added 3 commits
July 7, 2023 13:01
The access list protos have been cleaned up to fit into the existing generated protos a little more cleanly, and conversion functions have been migrated to their own packages and tests for them have been added.
mdwn
force-pushed
the
mike.wilson/access-list-proto-cleanup
branch
from
76c0996 to
90064d2
Compare
tcsc
reviewed
Jul 12, 2023
| "github.com/gravitational/teleport/lib/utils" | ||
| ) | ||
|
|
||
| // AccessListsGetter defines an interface for reading access lists. | ||
| type AccessListsGetter interface { | ||
| // GetAccessLists returns a list of all access lists. | ||
| GetAccessLists(context.Context) ([]*types.AccessList, error) | ||
| GetAccessLists(context.Context) ([]*accesslist.AccessList, error) |
There was a problem hiding this comment.
Not a huge fan of the name stuttering on the calling side here - but unfortunately I don't have a good suggestion for the AccessList type itself.
smallinsky
approved these changes
Jul 12, 2023
| } | ||
|
|
||
| // AccessListSpec is the specification for an access list. | ||
| type AccessListSpec struct { | ||
| // Spec is the specification for an access list. |
There was a problem hiding this comment.
What's the advantage of having almost duplicate structs here and generated structs from protobuf?
There was a problem hiding this comment.
There's a fairly substantial debate about it in #28386, but overall there's value in decoupling our internal storage representation from our API where applicable. In this case, I think it's applicable, so I've made this separate resource.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
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.
The access list protos have been cleaned up to fit into the existing generated protos a little more cleanly, and conversion functions have been migrated to their own packages and tests for them have been added.