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

decoder: Pick core schema only based on required_version constraint #993

Closed
2 tasks done
radeksimko opened this issue Jul 6, 2022 · 1 comment · Fixed by #1027
Closed
2 tasks done

decoder: Pick core schema only based on required_version constraint #993

radeksimko opened this issue Jul 6, 2022 · 1 comment · Fixed by #1027
Assignees
Labels
enhancement New feature or request modules Functionality related to the module block and modules generally
Milestone

Comments

@radeksimko
Copy link
Member

radeksimko commented Jul 6, 2022

#992 removes the other reason we run terraform version, so this issue may depend on it - at least we cannot avoid running it until we have other ways to get provider versions.

Background

Currently we pick core schema based on version that we determine by running terraform version -json via GetTerraformVersion job

func GetTerraformVersion(ctx context.Context, modStore *state.ModuleStore, modPath string) error {
mod, err := modStore.ModuleByPath(modPath)
if err != nil {
return err
}
err = modStore.SetTerraformVersionState(modPath, op.OpStateLoading)
if err != nil {
return err
}
defer modStore.SetTerraformVersionState(modPath, op.OpStateLoaded)
tfExec, err := TerraformExecutorForModule(ctx, mod.Path)
if err != nil {
sErr := modStore.UpdateTerraformVersion(modPath, nil, nil, err)
if err != nil {
return sErr
}
return err
}
v, pv, err := tfExec.Version(ctx)
pVersions := providerVersions(pv)
sErr := modStore.UpdateTerraformVersion(modPath, v, pVersions, err)
if sErr != nil {
return sErr
}
ipErr := modStore.UpdateInstalledProviders(modPath, pVersions)
if ipErr != nil {
return ipErr
}
return err
}

which we in turn run within every walked directory

id, err = idx.jobStore.EnqueueJob(job.Job{
Dir: modHandle,
Func: func(ctx context.Context) error {
ctx = exec.WithExecutorFactory(ctx, idx.tfExecFactory)
return module.GetTerraformVersion(ctx, idx.modStore, modHandle.Path())
},
Type: op.OpTypeGetTerraformVersion.String(),
})
if err != nil {
errs = multierror.Append(errs, err)
} else {
ids = append(ids, id)
}

This makes walking through a deep hierarchy with many modules quite an expensive process.

The scheduler already de-duplicates jobs (including GetTerraformVersion), such that we never run the same job for the same folder more than once, if it's already in the queue. This is a good barrier, but we anticipate the following features to put more pressure on the scheduler and create more situations of duplicate work (same job for the same folder) scheduled more sparsely, which the scheduler itself has no chance of catching.

both of which increase the chance of us scheduling the same jobs for the same directory repeatedly.

Proposal

  • terraform-schema: Introduce a function similar to the existing CoreModuleSchemaForVersion which takes version constraint instead of version and returns relevant root schema
    • Undeclared, empty constraint or constraint without upper bound can assume the latest TF version
  • decoder: Update logic responsible for picking core schema
    if mod.TerraformVersion != nil {
    var err error
    coreSchema, err = tfschema.CoreModuleSchemaForVersion(mod.TerraformVersion)
    if err != nil {
    return nil, err
    }
    coreRequirements, err = version.NewConstraint(mod.TerraformVersion.String())
    if err != nil {
    return nil, err
    }
    } else {
    coreSchema = tfschema.UniversalCoreModuleSchema()
    }

Notes

While running terraform version for every walked directory is expensive, there's still value in knowing Terraform version.

  • more accurate IntelliSense (esp. relevant if users don't use version constraints at all)
    • e.g. (assuming versioned docs) we can link to docs relevant to that particular version, not just first or last version where a language construct was present
  • future UX improvements nudging to upgrade TF

We should therefore retain the following logic in textDocument/didOpen:

if mod.TerraformVersionState == op.OpStateUnknown {
jobId, err := svc.stateStore.JobStore.EnqueueJob(job.Job{
Dir: modHandle,
Func: func(ctx context.Context) error {
ctx = exec.WithExecutorFactory(ctx, svc.tfExecFactory)
return module.GetTerraformVersion(ctx, svc.modStore, mod.Path)
},
Type: op.OpTypeGetTerraformVersion.String(),
})
if err != nil {
return err
}
jobIds = append(jobIds, jobId)
}

i.e. continue obtaining TF version when the user opens a module in the editor and we don't know the version yet.

@github-actions
Copy link

github-actions bot commented Sep 9, 2022

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, 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 Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request modules Functionality related to the module block and modules generally
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant