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

[Fix] Adds check to ensure that the Grafana-Terraform-Provider header is set #1631

Merged
merged 10 commits into from
Jun 13, 2024

Conversation

elainevuong
Copy link
Contributor

@elainevuong elainevuong commented Jun 11, 2024

To prevent regression due to missing request headers from the Terraform Provider, we add a check in the TF Provider Tests that ensures that following a successful CREATE or UPDATE, that a SLO returned from the READ call to the SLO API has a provenance field of terraform.

This is done directly in the TF Provider Tests resource_slo_test and does NOT affect the actual function of the Terraform Provider, which means it does not interfere with IMPORT functionality.

Fixes #1630

@elainevuong elainevuong requested review from a team as code owners June 11, 2024 21:56
Copy link

In order to lower resource usage and have a faster runtime, PRs will not run Cloud tests automatically.
To do so, a Grafana Labs employee must trigger the cloud acceptance tests workflow manually.

Copy link
Member

@julienduchesne julienduchesne left a comment

Choose a reason for hiding this comment

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

This prevents someone from importing a SLO and taking it over with Terraform

@ellisda
Copy link
Contributor

ellisda commented Jun 12, 2024

This prevents someone from importing a SLO and taking it over with Terraform

Is that because the resourceSloRead function is called during import as well? Is there a unit or integration test that we can add such a check to, outside of the production code path?

@julienduchesne
Copy link
Member

Yep, read is called on imports. The only way to test this would be to have a new acceptance test that creates a SLO without the header and then imports it (plenty of import examples in other TF tests)

@elainevuong
Copy link
Contributor Author

elainevuong commented Jun 12, 2024

I'll update this - and add the test @julienduchesne suggested - a new acceptance test that creates a SLO without the header and then imports it. thanks for the comments!

ended up doing this check directly in the TF Provider Tests, and not in the Provider itself so it shouldn't affect the Import functionality. doing the import tests took a bit more time to think through - I think it would require changes to the TF SLO Schema, since we don't currently have the readOnly field included in our SLO Schema anywhere. decided to just test for the header directly in our TF Provider Tests instead

@julienduchesne julienduchesne merged commit 2766bd3 into main Jun 13, 2024
27 checks passed
@julienduchesne julienduchesne deleted the ev/check-tf-provenance-header branch June 13, 2024 16:15
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.

[Fix] Returns an Error if the Grafana-Terraform-Provider Header is missing for Requests to the SLO API
3 participants