-
Notifications
You must be signed in to change notification settings - Fork 31
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
Update provider.go to handle slash at api_key_endpoint #568
Update provider.go to handle slash at api_key_endpoint #568
Conversation
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 contribution @akmal-spacelift! It looks like there's a slight issue with the code causing it to not compile. I've left some suggestions.
spacelift/provider.go
Outdated
|
||
// Handle / at api_key_endpoint | ||
endpoint := d.Get("api_key_endpoint").(string) | ||
if endpoint[len(api_key_endpoint)-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.
This would need to be len(endpoint)
- there's no api_key_endpoint
variable.
But I think I'd go with strings.TrimSuffix
here and then you don't need to worry about indexing. So something like:
endpoint := strings.TrimSuffix(d.Get("api_key_endpoint").(string), "/")
endpiont = fmt.Sprintf("%s/graphql", endpoint)
Also, can you test this locally please and confirm that it works with both cases? We have info in the readme about building a local copy of the provider and testing it: https://github.com/spacelift-io/terraform-provider-spacelift?tab=readme-ov-file#using-a-local-build-of-the-provider.
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.
Definitely! Thanks Adam
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 it locally both with and without the / at the end and it worked. I've also added a // at the end and it failed as expected.
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 Adam said 😛
Thanks @akmal-spacelift for fixing this! I've opened a new PR with your changes (the issue is that some of our PR checks don't run if you don't have write access to the repo) here: #568. I'm going to close this PR in favour of that one. |
Description of the change
Type of change
Related issues
Checklists
Development
false
.)go generate
to make sure the docs are up to dateCode review