-
Notifications
You must be signed in to change notification settings - Fork 1
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 Revisions #34
API Revisions #34
Conversation
config.go
Outdated
// An Aserto tenant ID. | ||
TenantID string `json:"tenant_id"` | ||
|
||
// An aserto account ID. |
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.
Aserto (uppercase)
ds/v2/directory.go
Outdated
@@ -36,7 +36,7 @@ func New(opts ...aserto.ConnectionOption) (*Client, error) { | |||
return nil, err | |||
} | |||
|
|||
if options.ServerAddress() == "" { | |||
if options.Address == "" { |
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.
Do we still need to v2 directory?
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.
You're right. I'll get rid of it.
config.go
Outdated
return nil, errors.Wrap(ErrInvalidConfig, "api_key and token are mutually exclusive") | ||
} | ||
|
||
if cfg.Insecure && cfg.NoTLS { |
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.
Do we need a mutually exclusive check for mTLS and (insecure | NoTLS)?
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 catch!
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.
See comments inline
- Need for directory v2, I would propose removing it, as topaz 32 no longer supports, and this go-aserto release will be a major version rev.
- Need for mTLS mutually exclusive check with Insecure or NoTLS
This PR includes breaking changes! It would be a minor version bump.o
SessionID
DialOptionsProvider
and roll its functionality intoConnectionOptions
.Config
inConnectionOptions
to eliminate duplication.ConnectionOptions
.WithClientCert()
.WithAccountID
and its equivalent config option.WithNoTLS
and its equivalent config option.