Skip to content
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

feat(iam): replace account on init command #2778

Merged
merged 19 commits into from
Feb 22, 2023

Conversation

Monitob
Copy link
Contributor

@Monitob Monitob commented Feb 15, 2023

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

Release note for CHANGELOG:


@Monitob Monitob marked this pull request as draft February 15, 2023 16:57
@Monitob Monitob force-pushed the feat-replace-account-v1-on-init branch from 073be65 to 2d09007 Compare February 20, 2023 14:01
@Monitob Monitob force-pushed the feat-replace-account-v1-on-init branch from 2d09007 to 0d70000 Compare February 20, 2023 14:22
@Monitob Monitob force-pushed the feat-replace-account-v1-on-init branch from 5310fec to 0723af1 Compare February 20, 2023 15:23
@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2023

Codecov Report

Merging #2778 (97449f9) into master (d628a49) will increase coverage by 0.02%.
The diff coverage is 43.38%.

@@            Coverage Diff             @@
##           master    #2778      +/-   ##
==========================================
+ Coverage   75.15%   75.18%   +0.02%     
==========================================
  Files         134      137       +3     
  Lines       26083    27805    +1722     
==========================================
+ Hits        19603    20904    +1301     
- Misses       5810     6186     +376     
- Partials      670      715      +45     
Impacted Files Coverage Δ
internal/core/context.go 26.59% <ø> (ø)
internal/core/validate.go 44.21% <0.00%> (-3.58%) ⬇️
internal/namespaces/iam/v1alpha1/error.go 0.00% <0.00%> (ø)
internal/namespaces/init/init.go 46.96% <35.92%> (-6.37%) ⬇️
internal/namespaces/iam/v1alpha1/custom.go 62.13% <69.23%> (ø)
internal/namespaces/iam/v1alpha1/iam_cli.go 80.13% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Monitob Monitob marked this pull request as ready for review February 20, 2023 15:35
@@ -9,6 +9,7 @@ AVAILABLE COMMANDS:
create Create an SSH key
delete Delete an SSH key
get Get an SSH key
init Initialize SSH key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't mean anything. Could you rephrase to something like, scan for ssh keys on your local machine and send them to your scaleway account.

@@ -0,0 +1,15 @@
🎲🎲🎲 EXIT CODE: 0 🎲🎲🎲
🟥🟥🟥 STDERR️️ 🟥🟥🟥️
Initialize SSH key.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to chance the phrasing here.

@@ -4,7 +4,7 @@ USAGE:
scw <command>

AVAILABLE COMMANDS:
account Account API
account User related data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change the phrasing here too.

@@ -54,7 +53,6 @@ func GetCommands(beta ...bool) *core.Commands {
marketplace.GetCommands(),
initNamespace.GetCommands(),
configNamespace.GetCommands(),
account.GetCommands(),
accountv2.GetCommands(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have one account. Rename it to simply account or avoid renaming entirely if possible.

@@ -736,6 +737,18 @@ scw iam ssh-key get <ssh-key-id ...> [arg=value ...]



### Initialize SSH key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change phrasing

"io"
"net/http"
)

// Token represents a Token
type Token struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is token maintained in this package? Should we use only the one in the IAM package?

@@ -16,11 +16,3 @@ var contextKey = contextKeyType{}
func InjectHTTPClient(ctx context.Context, httpClient *http.Client) context.Context {
return context.WithValue(ctx, contextKey, httpClient)
}

func extractHTTPClient(ctx context.Context) *http.Client {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems weird that we have to delete this code. Why? Why does using IAM instead of accountv2 impact this function?

var shortenedFilename string
var err error
var localSSHKeyContent []byte
for _, keyName := range [3]string{"id_ecdsa.pub", "id_ed25519.pub", "id_rsa.pub"} {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract this in a dedicated variable

@@ -150,6 +155,14 @@ Default path for configuration file is based on the following priority order:
}
}

if args.AccessKey == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed?

@@ -342,6 +358,38 @@ func promptSecret(ctx context.Context) (string, error) {
}
}

func promptAccessKey(ctx context.Context) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change required? access key can be found once you got the secret key.

@Codelax Codelax mentioned this pull request Feb 22, 2023
@Codelax Codelax added this pull request to the merge queue Feb 22, 2023
Merged via the queue into scaleway:master with commit dd3ef27 Feb 22, 2023
@remyleone remyleone linked an issue Mar 8, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace account ssh-key by iam ssh-key
5 participants