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

Fix code to open Terraform resource documentation #67

Merged
merged 13 commits into from
Mar 18, 2024

Conversation

dod38fr
Copy link
Contributor

@dod38fr dod38fr commented Oct 19, 2023

Currently the feature to open documentation is not always working. It relies on the output of terraform providers command. With AWS, this command works only with a valid token. This is a PITA when the token has a short life.

With this PR, the provider source is found by parsing current buffer and all terraform files for a required_providers declaration to find the provider source and build the documentation URL.

If the provider source is not found, this PR falls back on calling terraform providers like before.

One caveat: the required_providers declaration is searched with some regexps, so it's not bulletproof. Using a HCL parser or lsp would be better, but it's beyond my current skill set.

Copy link
Collaborator

@Fuco1 Fuco1 left a comment

Choose a reason for hiding this comment

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

Thanks for the feature/fix.

This was originally very simple and now it's about 50 lines of code to implement. It's getting a bit too complex 😊

There's some style suggestions and maybe some places the logic can be simplified, let's take it step by step.

Comment on lines +184 to +196
# blah blah
terraform {
required_providers {
aws = {
source = \"hashicorp/aws\"
version = \"~> 5\"
}
azurerm = {
source = \"hashicorp/azurerm\"
version = \"~> 3.47\"
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we should probably put some different content so it really loads the data from the file and not from the buffer (as a fallback).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loading provider source from this file is done in test command--terraform--get-resource-provider-source-provider-in-file line 201.

(defun terraform--get-resource-provider-source (provider &optional dir)
"Return provider source for PROVIDER located in DIR."
(goto-char (point-min))
(let ((source) (file) (file_path) (tf_files))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Elisp never uses the underscore, traditional style is to use a dash to separate words.

Further, if you are only declaring variables, they can be written in a single paren, (let (a b c d) ... ). They will all be set to nil.

But it's better to try to always declare and initialize at the same time rather than use setq later (kind of pretending we have immutability)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok for the first 2 points.

Originally, I did set source with (let ...). But the other *file* variables can only be set if there are some files to read, which is not the case if the user has just started to write the terraform file and has not yet saved his buffer.

All in all, I found awkward to set source in (let...), because it would be the only variable set this way, and it could be overridden later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On 2nd thought, I've improved the code following your suggestion. Only tf-files is not setup in let

@@ -252,18 +252,54 @@
(when (re-search-forward (concat "/\\(.*?\\)/" provider "\\]") nil t)
(match-string 1)))))

(defun terraform--get-resource-provider-source (provider &optional dir)
"Return provider source for PROVIDER located in DIR."
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not clear what the source is. Is it buffers, files, file names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll add these details in the doc.

Comment on lines 291 to 302
(if (= (length provider-source) 0)
;; fallback to old method with terraform providers command
(setq provider-source
(concat
(terraform--get-resource-provider-namespace provider)
"/" provider)))
(if (> (length provider-source) 0)
(format "https://registry.terraform.io/providers/%s/latest/docs/%s/%s"
provider-source
doc-dir
resource-name)
(user-error "Can not determine the provider namespace for %s" provider))))
(user-error "Can not determine the provider source for %s" provider))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is a bit too complicated. It's preferable not to re-assign variables in elisp if possible.

When using only the then of the if, use when that will make the intent a bit clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since there's 3 ways to set provider-source which are attempted, I don't know how to avoid re-assigning it. Any suggestion ?

@dod38fr dod38fr requested a review from Fuco1 October 29, 2023 17:56
@dod38fr
Copy link
Contributor Author

dod38fr commented Nov 14, 2023

@Fuco1 Any news ?

This function used to fail when a buffer is not saved. The message
was something like:

let*: Opening input file: Aucun fichier ou dossier de ce type, /home/domi/private/freelance/seira/repos/infra-terraform/modules/terragrunt/azure-config/.#cluster.tf

With this patch, terraform files name must be made with alphanum -
space and _ and finish with .tf
@Fuco1
Copy link
Collaborator

Fuco1 commented Mar 18, 2024

Thank you @dod38fr for following up on all my suggestions. I am very grateful for your contribution and I'm sorry it took me so long to review and merge this.

@Fuco1 Fuco1 merged commit f16a7ff into hcl-emacs:master Mar 18, 2024
3 checks passed
@dod38fr
Copy link
Contributor Author

dod38fr commented Mar 19, 2024

No worry. I'm glad it's been merged.

All the best

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.

2 participants