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

docs: make provider schema gen deterministic #86

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

bendrucker
Copy link
Contributor

Fixes a seemingly non-deterministic issue with provider docs generation, which caused the customer attribute to move from Required to Optional.

Background

We’ve observed that running go generate produces different results:

https://github.com/observeinc/terraform-provider-observe/blob/master/docs/index.md#schema

Sometimes customer, the one required attribute, is rendered in the Optional section. There's a GitHub Actions workflow that runs go generate && git diff and fails. The user must then revert this (incorrect) change.

So we need to identify the source of this non-determinism.

Analysis

I was able to identify the root cause via rubber ducking with Konstantin. He mentioned that trying a different terminal produced the expected result and that he’d set OBSERVE_* credentials in the first one. Then it clicked for me.

tfplugindocs works by using the terraform providers schema command to get a JSON representation of the provider’s schema:

https://developer.hashicorp.com/terraform/cli/commands/providers/schema#block-representation

Notably nowhere in this serialized schema is a Default. And that’s done because defaults can be computed at runtime. This is used to allow providers to read defaults from environment variables.

DefaultFunc: schema.EnvDefaultFunc("OBSERVE_CUSTOMER", nil),

And so it turns out that Terraform will return optional: true for that attribute.

if set to true, specifies that an omitted or null value is permitted.

In that sense, the attribute is optional, since an omitted (at least from the configuration) value is permitted.

Solution

Using env -u unsets the relevant environment variable. I'd prefer --unset but that's not portable to macOS's version of env. We could use a script to discover all OBSERVE_* variables and construct unset arguments, but this would be more than a one-liner. We cannot use env -i (prevents all env inheritance) because that breaks things like PATH which can prevents go and terraform from being found.

I also left a comment alongside the attribute itself in hopes that when someone refactors this to url or similar instead of customer and domain they'll notice that similar treatment is needed for their new environment variable.

Related

https://observe.atlassian.net/browse/OB-26565

Copy link
Contributor

@obs-gh-konstantintikhonov obs-gh-konstantintikhonov left a comment

Choose a reason for hiding this comment

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

Nice fix and analysis!

@bendrucker bendrucker merged commit acb0033 into master Dec 12, 2023
5 checks passed
@bendrucker bendrucker deleted the tfplugindocs-deterministic branch December 12, 2023 18:09
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.

2 participants