-
Notifications
You must be signed in to change notification settings - Fork 458
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
Api Refactor: Implement List Agents #1674
Api Refactor: Implement List Agents #1674
Conversation
139da8b
to
ce7d1f0
Compare
Id: ProtoFromID(spiffeID), | ||
X509SvidExpiresAt: n.CertNotAfter, | ||
X509SvidSerialNumber: n.CertSerialNumber, | ||
Banned: n.CertSerialNumber == "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a function to detect if an agent is banned or not in the BanAgent PR.
If that PR get merged first, maybe we could use that method here: nodeutil.IsAgentBanned(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I was waiting for that merge to use it
tx, err = applyPagination(p, tx) | ||
func listAttestedNodes(ctx context.Context, db *sqlDB, req *datastore.ListAttestedNodesRequest) (*datastore.ListAttestedNodesResponse, error) { | ||
if req.Pagination != nil && req.Pagination.PageSize == 0 { | ||
return nil, status.Error(codes.InvalidArgument, "cannot paginate with pagesize = 0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, status.Error(codes.InvalidArgument, "cannot paginate with pagesize = 0") | |
return nil, status.Error(codes.InvalidArgument, "cannot paginate with zero length page size") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have the same message for all pagination uses, may we update all of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with one nit
_, err = s.ds.CreateAttestedNode(ctx, &datastore.CreateAttestedNodeRequest{Node: aNode5}) | ||
s.Require().NoError(err) | ||
|
||
aNode1WithSelectors := proto.Clone(aNode1).(*common.AttestedNode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add helper function to clone attested node. It minimizes the number of type assertions one has to look at :)
Thanks for taking this on @MarcosDY :) That SQL plugin is a monster to work with :) |
- Implement list agent - Refactor datastore.ListAttestedNodes to allow filtering and returns selectors on demand. Signed-off-by: Marcos Yacob <marcos@scytale.io>
a904b07
to
88ae22c
Compare
selectors on demand.
Which issue this PR fixes
fixes #1634