-
Notifications
You must be signed in to change notification settings - Fork 9
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
CLI gating of commands and options by tier #882
base: main
Are you sure you want to change the base?
Conversation
f2bd0e0
to
47ba047
Compare
GetParametersByPath(ctx context.Context, params *ssm.GetParametersByPathInput, optFns ...func(*ssm.Options)) (*ssm.GetParametersByPathOutput, error) | ||
} | ||
|
||
var longRetryCfg = PanicOnError(config.LoadDefaultConfig(context.Background(), |
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 suspect this might remove all the nice error messages we've added when region etc. are not set and cause a panic instead.
src/cmd/cli/command/commands.go
Outdated
@@ -49,6 +50,7 @@ var ( | |||
nonInteractive = !hasTty | |||
providerID = cliClient.ProviderID(pkg.Getenv("DEFANG_PROVIDER", "auto")) | |||
verbose = false | |||
whoami *defangv1.WhoAmIResponse |
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.
Feels like this could be something that's inside the GrpcClient
|
||
if StsClient == nil { | ||
cfg := aws.PanicOnError(b.driver.LoadConfig(ctx)) | ||
StsClient = sts.NewFromConfig(cfg) |
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.
Not liking the panic
. What would it look like if we'd add the stsClient
pointer to the ByocAws
structure? And change the old code to "create if not initialized"?
{ | ||
name: "no permission for gpu", | ||
action: ActionRequest{ | ||
role: defangv1.SubscriptionTier_PERSONAL, |
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.
naming. tier <> role
role: defangv1.SubscriptionTier_PRO, | ||
action: "compose-up", | ||
resource: "gpu", | ||
detail: "1", |
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 already have a quota structure, so kinda expected us to leverage, extend, or replace that one.
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.
True, this is redundant and easily lead to inconsistency. Will make this reference the quota struct then.
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.
not sure "permission" is the right term, since it feels like it's an auth issue.
…ible everywhere permissions need to be checked.
Description
Add permission checking in CLI to give user's early feedback on whether they can execute certain commands or not.
This is an in-progress DRAFT.
Linked Issues
#874
Checklist