-
Notifications
You must be signed in to change notification settings - Fork 16
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
Implement Galadriel Server Admin CLI - GLCP-39474 #164
Conversation
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.
Good work, left some comments.
cmd/server/util/client.go
Outdated
return nil, fmt.Errorf(errReadResponseBody, err) | ||
} | ||
|
||
if res.StatusCode != http.StatusOK && res.StatusCode != http.StatusCreated { |
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.
this should be at the beginning of the function and return early if the statuscode is not success.
cmd/server/util/client.go
Outdated
return relationship, nil | ||
} | ||
|
||
func readBodyAndStatusCode(res *http.Response) ([]byte, error) { |
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.
rename to readResponse
cmd/server/cli/list.go
Outdated
if err != nil { | ||
return err | ||
} | ||
// Use: "trustdomains", |
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.
remove all the commented code
pkg/server/endpoints/admin.go
Outdated
@@ -288,7 +291,7 @@ func (h *AdminAPIHandlers) GetJoinToken(echoCtx echo.Context, trustDomainName ap | |||
} | |||
|
|||
if td == nil { | |||
errMsg := fmt.Errorf("trust domain exists: %q", trustDomainName) | |||
errMsg := fmt.Errorf("trust domain '%s' does not exists", trustDomainName) |
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.
use %q
for quoted strings
pkg/server/endpoints/admin_test.go
Outdated
@@ -483,7 +483,7 @@ func TestUDSGetJoinToken(t *testing.T) { | |||
echoHttpErr := err.(*echo.HTTPError) | |||
assert.Equal(t, http.StatusBadRequest, echoHttpErr.Code) | |||
|
|||
expectedMsg := fmt.Errorf("trust domain exists: %q", td1) | |||
expectedMsg := fmt.Errorf("trust domain '%s' does not exists", td1) |
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.
use %q
for quoted strings
pkg/server/endpoints/admin.go
Outdated
if err != nil { | ||
msg := errors.New("error looking up trust domain") | ||
errMsg := fmt.Errorf("%s: %w", msg, err) | ||
return nil, h.handleAndLog(errMsg, http.StatusInternalServerError) | ||
} | ||
|
||
if td == nil { | ||
errMsg := fmt.Errorf("trust domain exists: %q", trustDomainID) | ||
errMsg := fmt.Errorf("trust domain does not exists") |
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.
errMsg := fmt.Errorf("trust domain does not exists") | |
errMsg := fmt.Errorf("trust domain %q does not exists", tdName.String()) |
pkg/server/endpoints/admin.go
Outdated
func (h *AdminAPIHandlers) lookupTrustDomain(ctx context.Context, trustDomainID uuid.UUID, code int) (*entity.TrustDomain, error) { | ||
td, err := h.Datastore.FindTrustDomainByID(ctx, trustDomainID) | ||
func (h *AdminAPIHandlers) lookupTrustDomain(ctx context.Context, trustDomainName api.TrustDomainName, code int) (*entity.TrustDomain, error) { | ||
tdName := spiffeid.RequireTrustDomainFromString(trustDomainName) |
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.
don't use RequireTrustDomainFromString
, it's a method for testing, it panics instead of returning an error.
pkg/server/api/admin/helper.go
Outdated
@@ -9,8 +9,8 @@ import ( | |||
|
|||
func (r RelationshipRequest) ToEntity() *entity.Relationship { | |||
return &entity.Relationship{ | |||
TrustDomainAID: r.TrustDomainAId, | |||
TrustDomainBID: r.TrustDomainBId, | |||
TrustDomainAName: spiffeid.RequireTrustDomainFromString(r.TrustDomainAName), |
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.
don't use RequireTrustDomainFromString
pkg/server/api/admin/helper.go
Outdated
TrustDomainAID: r.TrustDomainAId, | ||
TrustDomainBID: r.TrustDomainBId, | ||
TrustDomainAName: spiffeid.RequireTrustDomainFromString(r.TrustDomainAName), | ||
TrustDomainBName: spiffeid.RequireTrustDomainFromString(r.TrustDomainBName), |
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.
same
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
Thanks @mgbcaio!
The rest of the tests and the CLI implementation will be added in a separate PR.
Approving.
Pull request check list
Affected functionality
Galadriel Server CLI:
generate token
,create trustdomain
andcreate relationship
Description of change
The galadriel Server CLI now uses the Admin API, through the
admin.Client
, to perform the requests regarding Trust Domains, Relationships and Join Tokens.WIP: List trust domains and relationships.
Renaming
management.go
andmanagement_test.go
->admin.go
andadmin_test.go
Some changes to
admin.yaml
that were kinda of when implementing the CLI to use the generated codeSome bug fixing regarding the CLI usage and output of the functions.
Which issue this pull requests fixes
GLCP-39474