microsoft / vscode-dev-containers

NOTE: Most of the contents of this repository have been migrated to the new devcontainers GitHub org (https://github.com/devcontainers). See https://github.com/devcontainers/template-starter and https://github.com/devcontainers/feature-starter for information on creating your own!
https://aka.ms/vscode-remote
MIT License
4.72k stars 1.41k forks source link

Update Azure Terraform dev container HashiCorp Extension settings #1608

Closed jpogran closed 2 years ago

jpogran commented 2 years ago

Currently the Azure Terraform devcontainer breaks the HashiCorp Extension by contributing incorrect default settings:

https://github.com/microsoft/vscode-dev-containers/blob/c0dcc9e14fae2498ad8e7ec8e5d21a1c8b72a079/containers/azure-terraform/.devcontainer/devcontainer.json#L29-L35

The terraform.languageServer.args property should not be empty, it should contain at least serve.

In addition, the HashiCorp Extension changed it's setting structure for some settings in v2.24.0. Old settings are still honored for a period of time, but we should update this devcontainer with the correct names.

Taking this all into account, the revised section should be:

"settings": {
  "terraform.languageServer.enable": true,
  "terraform.languageServer.args": [
    "serve"
  ]
}

Alternately, we can omit including these as they are the default values, but it might be safer to keep including them.

I can put up a PR with the changes, but found these settings referenced in a few other files in this repo. Should these be changed manually, or is there a process for those updates?

Chuxel commented 2 years ago

@jpogran We are in the process of restructuring as described in #1589 has been part of the issue here.

FWIW - part of that restructuring enables companies to self-publish "Dev Container Features". Certainly, it could make sense for Hashicorp to take over the current Terraform Dev Container Feature if that makes sense. In the future you'd then be able to update this as you saw fit. Let us know if that makes sense.

Tactically though we could update the contents here since the definition / template part of this has not yet been set up to enable self-publishing.

@bamurtaugh @joshspicer @samruddhikhandale - Looks like this would affect the post-move https://github.com/devcontainers/features/blob/main/src/terraform/devcontainer-feature.json as well.

bamurtaugh commented 2 years ago

@bamurtaugh @joshspicer @samruddhikhandale - Looks like this would affect the post-move https://github.com/devcontainers/features/blob/main/src/terraform/devcontainer-feature.json as well.

I just opened https://github.com/devcontainers/features/pull/124.

jpogran commented 2 years ago

Thank you @bamurtaugh and @Chuxel for the quick reply and modifications. While https://github.com/devcontainers/features/pull/124 does technically fix the broken default setting, I wasn't clear enough in my post to indicate that the settings should be the following:

Taking this all into account, the revised section should be:

"settings": {
  "terraform.languageServer.enable": true,
  "terraform.languageServer.args": [
    "serve"
  ]
}

Apologize for not catching this in your PR earlier, its been a busy week. LMK if you want me to put the PR up.

bamurtaugh commented 2 years ago

Hi @jpogran, please feel free to open a PR. Thanks!

jpogran commented 2 years ago

Done! Apologies for the back and forth.

samruddhikhandale commented 2 years ago

@jpogran Thanks for creating the PR, the Terraform Feature is published with your changes. 🚀 Could we close this issue if it looks good?

jpogran commented 2 years ago

Yep! Thank you so much for your help in getting this through!

samruddhikhandale commented 2 years ago

Closing as fixed.