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

add language HCL #359

Merged
merged 6 commits into from
Nov 27, 2024
Merged

add language HCL #359

merged 6 commits into from
Nov 27, 2024

Conversation

ppenguin
Copy link
Contributor

Terraform doesn't register hcl and doesn't offer good DX if manually set for editing e.g. nomad HCL files.

My first nvf contribution. Maybe somebody could show how to integrate a formatter as well. Other than that it's already an improvement for editing non-Terraform files (e.g. nomad), but it could be probably better. (I vaguely remember the vscode plugin offered some more features).

@ppenguin ppenguin requested a review from NotAShelf as a code owner August 17, 2024 16:52
@NotAShelf
Copy link
Owner

Looks reasonable. Though, I will need you to format via alejandra and not nixfmt. The formatting check is failing.

Other than that, if there is a known formatter for HCL I can work on getting it added tomorrow. We will need null-ls (or none-ls) to support HCL for proper formatting.

@ppenguin
Copy link
Contributor Author

Looks reasonable.

Thanks for looking into it so quickly...

Though, I will need you to format via alejandra and not nixfmt. The formatting check is failing.

Ah yes, different than nixpkgs 😅

While we're at it, is there a trick that I can (e.g. via direnv) use the active HM config for nvf but override certain attributes (like the formatter) in a devshell (when I'm in the nvf repo)? Or would I need to outsource my "standard" nvf config to a separate flake for that?

@ppenguin ppenguin force-pushed the add-hcl-not-terraform branch from 779e267 to 38d5869 Compare August 17, 2024 18:54
@NotAShelf
Copy link
Owner

NotAShelf commented Aug 17, 2024

Looking at none-ls builtins, I see an formatter source for Terraform. This could be used to add formatter support via terraform fmt. Refer to other language modules on how formatter submodules are added - the process is extremely straightforward but I'll be around if you have questions.

While we're at it, is there a trick that I can (e.g. via direnv) use the active HM config for nvf but override certain attributes (like the formatter) in a devshell (when I'm in the nvf repo)? Or would I need to outsource my "standard" nvf config to a separate flake for that?

Afraid there isn't a way to impurely override configuration per-project, but we do provide a formatter that will format the code for you with nix fmt. I will consider making this a pre-commit hook, or at least have pre-commit warn you before you commit the changes.

@ppenguin ppenguin marked this pull request as draft August 17, 2024 20:16
@NotAShelf NotAShelf added the enhancement New feature or request label Aug 18, 2024
Terraform doesn't register hcl and doesn't offer good DX if manually set
for editing e.g. nomad HCL files.

Incl. reformat with alejandra
@ppenguin ppenguin force-pushed the add-hcl-not-terraform branch from 38d5869 to 5952e60 Compare August 30, 2024 14:32
@ppenguin ppenguin marked this pull request as ready for review October 31, 2024 18:00
@ppenguin
Copy link
Contributor Author

ppenguin commented Oct 31, 2024

Looking at none-ls builtins, I see an formatter source for Terraform. This could be used to add formatter support via terraform fmt. Refer to other language modules on how formatter submodules are added - the process is extremely straightforward but I'll be around if you have questions.

I finally got around to finish this one. In hindsight I might have gone a bit overboard with (unnecessary?) extensibility (inspired by the bash setup), but given the fact that terraform-ls is a bit too generic for nomad, there may be more specific solutions in the future that we might then easily integrate.

EDIT: spelling (noticed spelling "funture", I might be on to something :rofl )

Copy link
Owner

@NotAShelf NotAShelf left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, GitHub never gave me any notifications about your comment.

Left a few comments, but nothing too big.

modules/plugins/languages/hcl.nix Outdated Show resolved Hide resolved
modules/plugins/languages/hcl.nix Outdated Show resolved Hide resolved
modules/plugins/languages/hcl.nix Show resolved Hide resolved
@ppenguin ppenguin force-pushed the add-hcl-not-terraform branch 2 times, most recently from 7489b91 to 4d4235e Compare November 12, 2024 15:45
@ppenguin ppenguin requested a review from NotAShelf November 23, 2024 17:51
@NotAShelf
Copy link
Owner

Sorry for the delay in my review, I'm very busy with IRL stuff at the moment.

We're currently at the final stretch for merging the next big release (see #355) and I've frozen the main branch to avoid merge conflicts. If you could rebase this around the v0.7 branch, I should be able to provide a final review or/and merge this PR tomorrow.

@ppenguin ppenguin force-pushed the add-hcl-not-terraform branch from 0d9ea35 to 3479b8d Compare November 26, 2024 10:26
@ppenguin
Copy link
Contributor Author

Thanks

If you could rebase this around the v0.7 branch, I should be able to provide a final review or/and merge this PR tomorrow.

Done (just pushed ther rebased one)

@ppenguin ppenguin changed the base branch from main to v0.7 November 26, 2024 10:29
Copy link
Owner

@NotAShelf NotAShelf left a comment

Choose a reason for hiding this comment

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

LGTM, but there seems to be an issue with the merge. Please pulling v0.7 with a rebase. There is a conflict between main (which is pulled) and v0.7 which you are targeting.

Also, a changelog entry wouldn't go amiss :)

@ppenguin ppenguin force-pushed the add-hcl-not-terraform branch from 3479b8d to 4bb7d05 Compare November 27, 2024 15:22
@ppenguin
Copy link
Contributor Author

Ah, the good old rebase vs merge mess 😉
Should be ok now...

consolidate changelog entry
@ppenguin ppenguin force-pushed the add-hcl-not-terraform branch from 7091c8b to 965da7d Compare November 27, 2024 15:31
@NotAShelf
Copy link
Owner

NotAShelf commented Nov 27, 2024

All good, merging once CI passes.

@NotAShelf NotAShelf merged commit 5087c3d into NotAShelf:v0.7 Nov 27, 2024
7 checks passed
@ppenguin ppenguin deleted the add-hcl-not-terraform branch November 27, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants