-
Notifications
You must be signed in to change notification settings - Fork 53
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: fallback to CLI config for token #248
Conversation
@Praveen005 is this ready for review? |
Yes @uzaxirr it is ready for review. |
@Praveen005 can you also build the provider locally, test it and attach corresponding screenshots. |
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've tested your branch locally. It works when credential_file
is specified or env vars are there but it fails when it has to fallback to CLI configs i.e when credential_file is not specified and env vars are also absent. can you please verify this.
civo/provider.go
Outdated
"token": { | ||
"credential_file": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
DefaultFunc: schema.EnvDefaultFunc("CIVO_TOKEN", ""), | ||
Description: "This is the Civo API token. Alternatively, this can also be specified using `CIVO_TOKEN` environment variable.", |
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.
Instead of removing token, we can deprecate this field from the schema. This would be a better way.
"token": {
Type: schema.TypeString,
Optional: true,
DefaultFunc: schema.EnvDefaultFunc("CIVO_TOKEN", ""),
Deprecated: "\"token\" attribute is deprecated. Moving forward, please use \"credential_file\" attribute.",
Description: "This is the Civo API token. Alternatively, this can also be specified using `CIVO_TOKEN` environment variable.",
},
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.
Hey @uzaxirr I zeroed in on the likely bottleneck,
The token
variable in provider.tf
in build.sh
script was still there, I removed it.
Also updated the provider.tf
in examples
directory.
I also incorporated your suggestion of deprecating the token field. Thanks for the heads up.
Following are the screen grabs from local build:
The resource didn't get created, likely because I don't have an active Civo Account.
Can you now try again?
Regards.
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.
While you performed the above operation was there any token
or credential_file attribute present in your provider.tf civo config block?
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.
@uzaxirr No, it was not in provider.tf
. I will share you the terraform debug log which shows token being read from ~/.civo.json
. The code need some more changes and I am working on that.
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.
Sure, great work so far
…ken field in provider schema
* Last commit was not reading from the token variable, so fixed that. * Added one test case to cater for above change. * Since, we still can read token from the token attribute, we need not remove civo_api_key variable from build.sh.
Hi @uzaxirr Thank you for your patience. Here are the details about the changes:
Screenshots:
![]()
![]()
![]()
![]()
![]()
![]()
![]()
![]()
![]()
![]()
![]()
![]() Can you please review these changes again? Regards. |
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.
Hey @Praveen005
I've pulled up your latest changes locally, again the same scenario works fine when crediential_file
or CIVO_TOKEN
env var is specified. but fails to obtain the token from CLI when nothing is specified. Please refer to the below screen shot
Also why do i see the Argument is deprecated
warning even though i haven't specified any token field in my schema.
I would suggest you to create a CIVO account, set up the CLI and then try spinning up resources via terraform with all the different scnerios.
I will check it right away, @uzaxirr. |
Added a validation function in token field, which will ensure warning is shown only when user uses the token variable. Removed hardcoded api key name. Made minor changes to unit tests
Hi @uzaxirr, sorry it took way more time than I anticipated. I believe, it fixes all the above issues you pointed. I have tested it thoroughly and it works fine, can you kindly check once from your end?
Screenshots:
![]() Network resource successfully created: ![]() |
@Praveen005 can you test the following locally and attach screenshots for them.
|
Thank you for reviewing it @uzaxirr. Here are all the relevant screenshots:
![]() ![]()
![]() ![]() ![]()
![]() ![]()
![]() ![]()
![]() Resource creation failed: ![]() |
Looks great to me so far. As a suggested improvement, if no token is found at all. We could point the user to where to get one: https://dashboard.civo.com/security For example: "No token configuration found in $CIVO_TOKEN or ~/.civo.json, please go to https://dashboard.civo.com/security to fetch one" |
LGTM! |
I hope you're doing well. When you have a moment, could you please review this PR? Thank you! |
hey @Praveen005 the PR looks fine, i'm in process of updating the corresponding docs for this before we merge off this one |
Thank you @uzaxirr. Let me know if there's anything I can assist with regarding the documentation. Regards! |
One more thing, when we have the updated documents, I would also add a link to the documentation in the deprecation message. Thank you for your hard work and patience @Praveen005 ! |
@fernando-villalba That would be better. Just a caveat, the link to the documentation will have to be added in ![]() Also, in the documentation, should we mention the format in which user should store their token in the credential file?
This way user can store multiple API keys for multiple accounts (home, work etc.). Also it would be consistent with the way, Civo CLI stores token in |
By the way, @Praveen005 and @uzaxirr Sorry to be a stickler for detail but change I know that technically we are only using one credential, the one token, but in a security context is weird referring to it as singular. Other than that, let's get this out on next release - doing documentation for it now! |
That is a lapse on my part, and I’ll be more mindful in future. Thanks for the heads up. I'll update the code to use Regards, Praveen |
@Praveen005 can you do the above change on priority so that i could merge this PR |
Hey @uzaxirr, I have tested it again and it works as intended. with ![]() with ![]() |
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.
Thankss a ton for this PR
Thank you @uzaxirr . It means a lot. |
This pull request introduces the feature requested in #246
It Implements a priority-based token retrieval:
Tests' Screenshots:
I have introduced coloured debug messages(removed from the main code), which correctly prints the token it abstracts:
Below, I have set the token using Civo CLI command
civo apikey save CIVO_TOKEN clitoken96
which saves it in~/.civo.json
.You can see, it correctly prints a different token(last coloured debug message):