-
Notifications
You must be signed in to change notification settings - Fork 100
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
Adding rad init command changes to support irsa #7761
Adding rad init command changes to support irsa #7761
Conversation
Signed-off-by: Vishwanath Hiremath <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7761 +/- ##
==========================================
- Coverage 61.05% 61.04% -0.02%
==========================================
Files 523 523
Lines 27367 27432 +65
==========================================
+ Hits 16710 16745 +35
- Misses 9185 9207 +22
- Partials 1472 1480 +8 ☔ View full report in Codecov by Sentry. |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
pkg/cli/aws/provider.go
Outdated
// AwsCredentialKind - Aws credential kinds supported. | ||
type AwsCredentialKind string |
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.
What is the convention in our repository for Aws
? Should it be AWS
or do we use both Aws
and AWS
? I believe we should use AWS
.
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 see we are using mix of these 2 formats, mostly i see Aws in ucp code and AWS on the cli side. I have updated it to AWS format
pkg/cli/cmd/radinit/aws.go
Outdated
selectAWSRegionPrompt = "Select the region you would like to deploy AWS resources to:" | ||
selectAwsCredentialKindPrompt = "Select a credential kind for the AWS credential:" |
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 use AWS
and Aws
in two lines that follow each other.
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.
updated it
return nil, err | ||
} | ||
|
||
// Set the value for the Helm chart |
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 don't need these comments in production, I guess.
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.
today i see comments in azure provider section as well, we can remove it if its not recommended https://github.com/vishwahiremat/radius/blob/b7484430568763f05fba769b325e6f0471092fcb/pkg/cli/cmd/radinit/azure.go#L127-L128
pkg/cli/cmd/radinit/aws.go
Outdated
return r.Prompter.GetListInput(credentialKinds, selectAwsCredentialKindPrompt) | ||
} | ||
|
||
func (r *Runner) buildAwsCredentialKind() ([]string, 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.
This returns a list of available AWS credential kinds. Should we change the name? Also, can this ever return an 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.
updated it to remove error from the return type.
and for the name today we follow the similar format in azure provider:
radius/pkg/cli/cmd/radinit/azure.go
Line 307 in b748443
func (r *Runner) buildAzureCredentialKind() ([]string, error) { |
Signed-off-by: Vishwanath Hiremath <[email protected]>
return r.Prompter.GetListInput(credentialKinds, selectAWSCredentialKindPrompt) | ||
} | ||
|
||
func (r *Runner) buildAWSCredentialKind() []string { |
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 guess we can also change the name of the function. Are we building something here? Is this the convention we are using? Isn't it more like awsCredentialKinds
?
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 is format we used in azure, we are using the similar format in azure provider.
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
# Description - Added changes to `rad init --full` command to add types i.e accesskey,irsa while configuring aws provider  - And adding prompts to accept role arn - refactoring the code with switch cases to handle it differently for both cases. - updated the tests ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request fixes a bug in Radius and has an approved issue (issue link required). - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: #issue_number --------- Signed-off-by: Vishwanath Hiremath <[email protected]>
# Description - Added changes to `rad init --full` command to add types i.e accesskey,irsa while configuring aws provider  - And adding prompts to accept role arn - refactoring the code with switch cases to handle it differently for both cases. - updated the tests ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request fixes a bug in Radius and has an approved issue (issue link required). - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: #issue_number --------- Signed-off-by: Vishwanath Hiremath <[email protected]> Signed-off-by: Reshma Abdul Rahim <[email protected]>
Description
rad init --full
command to add types i.e accesskey,irsa while configuring aws providerType of change
Fixes: #issue_number