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

Splunk token required #579

Merged
merged 5 commits into from
Jul 20, 2022
Merged

Splunk token required #579

merged 5 commits into from
Jul 20, 2022

Conversation

Integralist
Copy link
Collaborator

Fixes #577

NOTES: The customer initially reported the Fastly API was returning an ambiguous error. The error messaging was intentional as the API internals are sharing an implementation across multiple types, which means the error had to be generic in wording.

To help avoid confusion a new version of go-fastly was released (v6.4.3) with additional checks for the Token field. This PR bumps the go-fastly dependency to the new version.

I then also noticed a bug in the Terraform provider where the use of schema.EnvDefaultFunc meant that if no environment variable (i.e. FASTLY_SPLUNK_TOKEN) had been set, then an empty string would be used. Returning an empty string causes Terraform to use the value as the fallback and sends the empty string to go-fastly.

Instead of an empty string, if the helper function returned nil, then Terraform would enforce the 'required field' behaviour on the Token attribute and would prompt the user to add a value to their HCL configuration. Thus avoiding having to even call out to go-fastly (only for go-fastly's new check for a token value to fail).

@Integralist Integralist requested review from a team and harmony7 and removed request for a team July 20, 2022 10:20
Copy link
Member

@kailan kailan left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Integralist Integralist mentioned this pull request Jul 20, 2022
I removed this test because although it worked locally, once running the full
test suite I quickly discovered that the pausing of a test (to be resumed later,
i.e. concurrent processing) was causing the environment to get very confused and
it wasn't adding a whole lot of value to be honest.
@Integralist Integralist force-pushed the integralist/token-required branch from 2d5109e to 0492c54 Compare July 20, 2022 12:44
@Integralist Integralist merged commit 29ccd3c into main Jul 20, 2022
@Integralist Integralist deleted the integralist/token-required branch July 20, 2022 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opaque 400 Error
2 participants