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

Allow configured providers to provide additional functions. #1491

Merged
merged 25 commits into from Apr 18, 2024

Conversation

cam72cam
Copy link
Contributor

@cam72cam cam72cam commented Apr 15, 2024

Prior to this change a single unconfigured lazy provider instance was used per provider type to supply functions. This used the functions provided by GetSchema only.

With this change, provider function calls are detected and supplied via GraphNodeReferencer and are used in the ProviderFunctionTransformer to add dependencies between referencers and the providers that supply their functions.

With that information EvalContextBuiltin can now assume that all providers that require configuration have been configured by the time a particular scope is requested. It can then use it's initialized providers to supply all requested functions.

At a high level, it allows providers to dynamically register functions based on their configurations.

main.lua

function main_a( input )
    local animal_sounds = {
        cat = 'meow',
        dog = 'woof',
        cow = 'moo'
    }
    return animal_sounds
end

main.tf

terraform {
  required_providers {
    tester = {
      source  = "terraform.local/local/testfunctions"
      version = "0.0.1"
    }
  }
}

provider "tester" {
        lua = file("./main.lua")
}

output "test" {
        value = provider::tester::main_a(tomap({"foo": {"bar": 190}}))
}

Output:

Changes to Outputs:
  + test = {
      + cat = "meow"
      + cow = "moo"
      + dog = "woof"
    }

This requires some enhancements to the HCL library and is currently pointing at my personal fork, with a PR into the main HCL repository: hashicorp/hcl#676. As this may take some time for the HCL Team to review, I will likely move the forked code under the OpenTofu umbrella before this is merged [done].

This PR will be accompanied by a simple provider framework for implementing function providers similar to the example above.

Related to #1326

Target Release

1.7.0

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Copy link

Reminder for the PR assignee: If this is a user-visible change, please update the changelog as part of the PR.

Copy link

Please link the relevant issue that this PR handles using one of the following words

  • close | closes | closed | fix | fixes | fixed | resolve | resolves | resolved if the Pull Request resolves the issue - more details can be found here
  • relates to | related to | part of if the Pull Request is just part of the solution

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Yantrio
Yantrio previously approved these changes Apr 15, 2024
Copy link
Member

@Yantrio Yantrio left a comment

Choose a reason for hiding this comment

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

Overall I like it. I'm tempted to say that we should have some sort of ProviderFunctionFromString(input string) ProviderFunction method that parses the addr, and then from there we can have a IsCore() method.

That could help the readability of the code.

LGTM so far, once the TODOs are done I'll be happy to review again. (Also we could do with some more tests)

internal/addrs/provider_function.go Outdated Show resolved Hide resolved
internal/lang/eval_test.go Show resolved Hide resolved
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
internal/addrs/parse_ref.go Outdated Show resolved Hide resolved
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
@cube2222
Copy link
Collaborator

WIP review: looking good!

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
This is because we need to attach provider schemas before
we can properly look for provider functions required by resources

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Copy link

Please link the relevant issue that this PR handles using one of the following words

  • close | closes | closed | fix | fixes | fixed | resolve | resolves | resolved if the Pull Request resolves the issue - more details can be found here
  • relates to | related to | part of if the Pull Request is just part of the solution

@cam72cam cam72cam requested a review from a team as a code owner April 16, 2024 19:51
@cam72cam cam72cam changed the title WIP: Allow configured providers to provide additional functions. Allow configured providers to provide additional functions. Apr 16, 2024
Copy link

Please link the relevant issue that this PR handles using one of the following words

  • close | closes | closed | fix | fixes | fixed | resolve | resolves | resolved if the Pull Request resolves the issue - more details can be found here
  • relates to | related to | part of if the Pull Request is just part of the solution

janosdebugs
janosdebugs previously approved these changes Apr 17, 2024
Copy link
Contributor

@janosdebugs janosdebugs left a comment

Choose a reason for hiding this comment

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

Looks ok, but I can't really judge the completeness of the code as I'm not familiar with most parts of it.

internal/tofu/transform_provider.go Show resolved Hide resolved
internal/lang/eval.go Outdated Show resolved Hide resolved
cube2222
cube2222 previously approved these changes Apr 17, 2024
Copy link
Collaborator

@cube2222 cube2222 left a comment

Choose a reason for hiding this comment

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

Left a couple nit comments.

LGTM overall!

Yantrio
Yantrio previously approved these changes Apr 17, 2024
Copy link
Member

@Yantrio Yantrio left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me. I have some concerns around the performance of the graph transformation stuff, but I would need to benchmark and run it to see how performant it is.

My comments are mostly small nitpicks, which I would prefer are addressed, but I'll leave that up to others to decide based on timings.

Nice work! 👍

go.sum Outdated Show resolved Hide resolved
internal/addrs/provider_function.go Outdated Show resolved Hide resolved
internal/lang/eval.go Outdated Show resolved Hide resolved
internal/lang/references.go Outdated Show resolved Hide resolved
internal/tofu/eval_context_builtin.go Outdated Show resolved Hide resolved
@Yantrio Yantrio mentioned this pull request Apr 17, 2024
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
@cam72cam cam72cam dismissed stale reviews from Yantrio, cube2222, and janosdebugs via c9ed866 April 17, 2024 13:49
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
cube2222
cube2222 previously approved these changes Apr 17, 2024
Copy link
Collaborator

@cube2222 cube2222 left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
@cube2222 cube2222 merged commit a69d19d into main Apr 18, 2024
8 checks passed
@cube2222 cube2222 deleted the graph_functions branch April 18, 2024 13:11
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.

None yet

4 participants