-
Notifications
You must be signed in to change notification settings - Fork 182
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
Added Scaleway Shell Plugin #314
base: main
Are you sure you want to change the base?
Conversation
@arunsathiya please help me to proceed |
Hello! Is this a submission for the 1Password Hackathon with Hashnode? If so, when you're ready, please be sure you write a blog post on Hashnode to make your submission official. Full instructions are on the Hackathon page. |
Thank you for information, I will write blog But can you please help me in this plugin |
Hi Parthiv, thanks for confirming. Yes, reviewing this PR is on my radar. I'll follow up soon! |
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.
Thank you for the contribution @parthiv11 . This PR is on the right track. Here's the documentation for the scale way CLI (for other reviewers) which let's us know that these env vars actually work: sdk/schema/credname/names.go
Did a first iteration and left the first suggestions. After you're done with those, feel free to move on to resolving the TODO comments you've added.
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.
Mostly nits, thank you for your contribution! ❤️
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 for the improvements! I think there are a few open comments from prior reviews, let's make sure those get addressed as well.
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.
Looks great! Only a few nits and ready to go IMO 👍
plugins/scaleway/api_key.go
Outdated
AccessKey string `yaml:"access_key"` | ||
SecretKey string `yaml:"secret_key"` | ||
DefaultOrganizationID string `yaml:"default_organization_id"` | ||
DefaultProjectID string `yaml:"default_project_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.
Is this attribute used anywhere?
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 noticed the same thing. @parthiv11, let's remove this since this is not being used anywhere. But before we do, would you know if the Scaleway CLI needs both the organization ID and the project ID to function?
Hey @parthiv11 , I see there hasn't been any activity on this PR in a bit, is there anything that we can help you with? We'd love to get this to the finish line together! 🚀 |
Sorry for delay, |
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 looks 95% done, thanks for your contribution @parthiv11! There's only one major task to complete here: support importing of multiple profiles from the config file. Could you give it a try? We'll be around if you need a hand. 😄
Name: "Scaleway CLI", | ||
Runs: []string{"scw"}, | ||
DocsURL: sdk.URL("https://www.scaleway.com/en/cli"), | ||
NeedsAuth: needsauth.IfAll( |
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.
Let's skip for -p
as well, because that applies only when the configuration file exists on the computer. So, it's not relevant when a shell plugin is being used.
plugins/scaleway/api_key.go
Outdated
AccessKey string `yaml:"access_key"` | ||
SecretKey string `yaml:"secret_key"` | ||
DefaultOrganizationID string `yaml:"default_organization_id"` | ||
DefaultProjectID string `yaml:"default_project_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.
I noticed the same thing. @parthiv11, let's remove this since this is not being used anywhere. But before we do, would you know if the Scaleway CLI needs both the organization ID and the project ID to function?
plugins/scaleway/api_key.go
Outdated
if config.DefaultZone != "" { | ||
fields[fieldname.DefaultZone] = config.DefaultZone | ||
} | ||
out.AddCandidate(sdk.ImportCandidate{ |
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.
It seems that Scaleway config file supports the concepts of profiles. Could we import the profile name as a Namehint? You'll need to set up at least two profiles to test the importer. Example:
profiles:
first:
access_key: redacted
secret_key: redacted
default_organization_id: redacted
default_project_id: redacted
default_zone: fr-par-1
default_region: fr-par
# api_url: https://api.scaleway.com
# insecure: false
second:
access_key: redacted
secret_key: redacted
default_organization_id: redacted
default_project_id: redacted
default_zone: fr-par-1
default_region: fr-par
# api_url: https://api.scaleway.com
# insecure: false
There's an example of how to import multiple profiles in the Fastly shell plugin.
Please have a look over your tests and lint 😄 You can run tests locally using |
@parthiv11 is there a way we can help you move this forward? |
@AndyTitu Not able to test it currently as I don't have scaleway account |
Overview
Used previous PR for this Scaleway Shell Plugin code
Created a new plugin
Related Issue(s)
How To Test
Changelog