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

Support unknown variable evaluation #413

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Support unknown variable evaluation #413

wants to merge 2 commits into from

Conversation

notnoop
Copy link
Contributor

@notnoop notnoop commented Oct 23, 2020

Expose a callback to handle unknown variables found in hcl syntax. This is useful in situations where the variables aren't known statically upfront.

When a chain of EvalContexts is used, the deepest child context with set UnknownVariable is used.

Expose a callback to handle unknown variables found in hcl syntax. This is
useful in situations where the variables aren't known statically upfront.
@apparentlymart
Copy link
Contributor

Hi @notnoop!

This seems reasonable to me in principle. I'd like to suggest a couple small tweaks to it in the hope of making it fit better with the existing features of this module:

  • The word "unknown variable" confused me a little at first because HCL (via cty) already has a special idea of a "unknown value", which is a variable that is explicitly defined as not being known yet. I'd prefer to call this new field UndefinedVariable or MissingVariable to avoid creating two meanings of the word "unknown" in this context.

  • In HCL, functions that might fail typically return hcl.Diagnostics instead of error, so that the function can return zero or more structured diagnostic messages rather than an opaque error objects. Since this callback function is called during expression evaluation, I'd prefer its signature to return (cty.Value, hcl.Diagnostics) and then return the diagnostics to the TraverseAbs caller.

    The typical pattern for that is for the function to start by declaring a zero-value Diagnostics and then gradually append new diagnostics to it as it goes:

    var diags Diagnostics
    // ...
    if unknownHandler != nil {
          v, moreDiags := unknownHandler(t)
          diags = append(diags, moreDiags...)
          return v, diags
    }
    //
    diags = diags.Append(&Diagnostic{
        Severity: DiagError,
        Summary:  "Unknown variable",
         // etc
    })
    return cty.DynamicVal, diags

    (Note that diags can be non-nil even if there are no errors, because it can potentially contain warnings too.)

Use UndefinedVariable to avoid the overloading usage of Unknown in the
library.
@notnoop
Copy link
Contributor Author

notnoop commented Jan 29, 2021

Thanks @apparentlymart for the feedback and sorry for taking long to get back. I have updated the PR to use UndefinedVariable.

I have opted not go through the diags accumulation approach. The function TraverseAbs never returns the accumulation of multiple diagnostics. If UndefinedVariables returns some error diagnostics, we probably should return those as is, without additional "did you mean ..." suggestions, showing multiple, maybe conflicting, error messages for the same variable reference will be confusing to users, I suspect.

Base automatically changed from hcl2 to main February 10, 2021 22:41
@hashicorp-cla
Copy link

hashicorp-cla commented Sep 9, 2021

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


Mahmood Ali seems not to be a GitHub user.
You need a GitHub account to be able to sign the CLA. If you already have a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

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

3 participants