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

Add functions definitions to the providers schema -json output #34450

Merged
merged 4 commits into from
Jan 10, 2024

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Dec 22, 2023

Add function definitions to the json schema CLI output. While the protocol names may not exactly match the json keys, this matches the existing metadata functions output for easier integration with tooling already using that interface. We use the jsonfunction package from the metadata command to serialize the new provider functions, and ensure they stay synchronized. This makes the integration a little clumsy, and it may be a good idea to combine the cli json definitions into a single package, but the separate serialization works well enough for now.

An example of the "functions" portion of the schema would look like:

      "functions": {
        "format_version": "1.0",
        "function_signatures": {
          "noop": {
            "description": "noop takes any single argument and returns the same value",
            "return_type": "dynamic",
            "parameters": [
              {
                "name": "noop",
                "description": "any value",
                "is_nullable": true,
                "type": "dynamic"
              }
            ]
          }
        }
      }
    },

The provider functions are not being added to the metadata functions command. The function names in the configuration are dependent on the local provider name in the module, and therefor are context dependent. The metadata command is also not guaranteed to have an initialized working directory and providers may not be available at all.

Provider functions should marshal identically to an equivalent cty
functions.
Add provider functions to the json schema output for providers. We
leverage the jsonfunction package here to ensure that the serialization
is kept identical, so that the consumers of the json function
definitions can work equally with both.
@jbardin jbardin requested review from bflad, radeksimko and a team December 22, 2023 15:08
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

While the protocol names may not exactly match the json keys

Can you expand on this? Is it just the whitespaces around :: that you're referring to here or is there some other reason for the mismatch?

It would be useful to have an example output, I expect we'll need it anyway for a docs page.

The function names in the configuration are dependent on the local provider name in the module, and therefor are context dependent.

Is there any value in embedding the local provider name in the output? i.e. are there any consumers who would benefit from it today?

So far the connection between configuration and terraform providers schema -json was only indirect, an implementation detail of how providers get installed. AFAICT In theory we could have a future providers schema -json which wholly relies on provider binaries and leaves the complexities of discovering and installing and choosing providers to init. This moves us further away from ever achieving that.

TL;DR Can we just use the last segment such as count_e from provider::aws::count_e in the JSON output?


In the context of Terraform LS:

We already avoid running terraform provider schemas -json most of the time at runtime for the exact reasons you mentioned - it requires a lot of context we don't have in an IDE. Also we frequently deal with invalid configuration which would make it fail anyway and finally it's very costly (CPU/memory/time) to run it, esp. because Terraform CLI will redo all the same things we have done, such as parsing and some early decoding of all files.

We will of course need to get the local name from somewhere but we already utilize a minimal decoder (think of it as a different implementation of terraform-config-inspect) to solve that exact problem.

Therefore we don't really care about the configuration at the time of running the command. In fact it just gets in the way as we do use that command as a fallback for unofficial/non-partner and other local-only providers.

In this context, it seems unlikely we would run the command again just because the local provider name has changed as running that command once and processing the output is already expensive as it is.

@jbardin
Copy link
Member Author

jbardin commented Jan 3, 2024

While the protocol names may not exactly match the json keys

Can you expand on this? Is it just the whitespaces around :: that you're referring to here or is there some other reason for the mismatch?

Simply the field names decided on for the protocol don't align with the existing keys used in the json function definitions, e.g. "is_nullable" !~ AllowNullValues

The function names in the configuration are dependent on the local provider name in the module, and therefor are context dependent.

Is there any value in embedding the local provider name in the output? i.e. are there any consumers who would benefit from it today?

The local provider name could be any valid identifier as defined within any module, and there may be many for any particular provider. Since that is dependent on the configuration, and schema is independent of the configuration, it feels more clear to simply state the schema and leave the scoping to the configuration.

So far the connection between configuration and terraform providers schema -json was only indirect, an implementation detail of how providers get installed. AFAICT In theory we could have a future providers schema -json which wholly relies on provider binaries and leaves the complexities of discovering and installing and choosing providers to init. This moves us further away from ever achieving that.

TL;DR Can we just use the last segment such as count_e from provider::aws::count_e in the JSON output?

Sorry, the json output is only the last segment which is the function name within the provider's schema, because the scoping of the function is not related to the schema. I'll add an example here shortly

@bflad
Copy link
Contributor

bflad commented Jan 5, 2024

Other than synchronizing the machine-readable outputs to make it somewhat easier for integrations, is there a better reason to synchronize the two? The provider-defined functions protocol was specifically designed for the additional needs of provider implementation details and documentation, I'm worried that the drift between the naming of things between the protocol and machine-readable output will ultimately be more confusing if/when we change it further. Being able to show fields like allow_unknown_values could improve documentation, for example, by having the documentation generation aware of that behavior. That information could also be useful in troubleshooting, since otherwise developers will need to know/show their function definition code and we'll have ensure the provider side implementation is always doing exactly what it should be doing (which hey, is a great goal we should always strive for, but reality changes over time).

Since our protocol definitions tend to remain fairly static, I think setting us up for less potential pain/confusion in the future is a beneficial move, over any potential ease benefits of integrations that need to reference both outputs.

@radeksimko
Copy link
Member

@bflad I'm not opposed to the provider definitions containing new fields, as long as the existing fields with the same meaning are called the same. That was my main concern in the original RFC that the two formats drift in naming.

That said I also wonder whether these additional fields could be relevant for core functions too, or why don't we expose it today? Is that something that you think would make documentation of Core functions better?

Of course the function docs today are still hand-maintained but I can imagine a future where they become generated too, so this would be IMO relevant question to ask.

@jbardin
Copy link
Member Author

jbardin commented Jan 5, 2024

If the names are diverging, but must be consumed via the same interface, then someone must translate the divergent structures. My position here was that since core is the point at which the names diverge, it should be responsible for that translation as well. Having a single structure to deal with all function output ensures they are always aligned with future changes too.

Both core and provider functions follow the same cty calling convention and have the same options. The reason that allow_unknown_values was not included originally, is that it is only an implementation detail and makes no difference to the end user which way it is set (it's always "allowed" in the config, it's just whether the call is made to the provider or resolved statically)

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

I don't have a provider handy with which I could test this but I'm choosing to trust the example output you attached reflects the implementation. 😄 i.e. only the function names without the :: separators and other segments are included in the output.

In which case LGTM, I have no further objections here.

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

I trust both your opinions here; my only request then would be to ensure https://developer.hashicorp.com/terraform/cli/commands/providers/schema gets updated as well. 👍

@bflad
Copy link
Contributor

bflad commented Jan 5, 2024

By the way, the function definition summary field is probably a better example. The existing jsonfunction handling doesn't handle that information.

@jbardin
Copy link
Member Author

jbardin commented Jan 5, 2024

Thanks @bflad, I'll follow up with an addition covering summary. That one is new to the protocol.

@jbardin jbardin merged commit b3c5590 into main Jan 10, 2024
6 checks passed
@jbardin jbardin deleted the jbardin/provider-functions-json-schema branch January 10, 2024 18:19
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

bflad added a commit that referenced this pull request Jan 11, 2024
bflad added a commit to hashicorp/terraform-json that referenced this pull request Jan 11, 2024
…-json

Reference: #118
Reference: hashicorp/terraform#34450

The Terraform `providers schema -json` output implementation currently uses the entire `metadata functions -json` implementation, so has the additional nesting layer containing `format_version` and `function_signatures`.
bflad added a commit that referenced this pull request Jan 12, 2024
radeksimko pushed a commit to hashicorp/terraform-json that referenced this pull request Jan 19, 2024
…-json (#119)

* Initial support for provider-defined functions from providers schema -json

Reference: #118
Reference: hashicorp/terraform#34450

The Terraform `providers schema -json` output implementation currently uses the entire `metadata functions -json` implementation, so has the additional nesting layer containing `format_version` and `function_signatures`.

* Remove extraneous function metadata object
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants