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

Please add nomad fmt command #11757

Closed
nahsi opened this issue Dec 30, 2021 · 13 comments · Fixed by #14779
Closed

Please add nomad fmt command #11757

nahsi opened this issue Dec 30, 2021 · 13 comments · Fixed by #14779
Assignees
Labels
good first issue help-wanted We encourage community PRs for these issues! stage/accepted Confirmed, and intend to work on. No timeline committment though. theme/cli theme/hcl type/enhancement
Milestone

Comments

@nahsi
Copy link

nahsi commented Dec 30, 2021

Please provide command fmt to format nomad jobs analogous to terraform fmt and packer fmt.

Right now I'm using hclfmt from hcl repo.

To install it I use this command go install github.com/hashicorp/hcl/v2/cmd/hclfmt@latest.

@jrasell jrasell added stage/accepted Confirmed, and intend to work on. No timeline committment though. theme/cli theme/hcl labels Jan 3, 2022
@jrasell jrasell added this to Needs Triage in Nomad - Community Issues Triage via automation Jan 3, 2022
@jrasell jrasell moved this from Needs Triage to Needs Roadmapping in Nomad - Community Issues Triage Jan 3, 2022
@jrasell
Copy link
Member

jrasell commented Jan 3, 2022

Hi @nahsi and thanks for raising this request. I believe this addition would be very useful!

@mikenomitch mikenomitch added the help-wanted We encourage community PRs for these issues! label Apr 1, 2022
@rounakdatta
Copy link

Hi @jrasell @mikenomitch is the nomad fmt command expected to be just a usage of hclfmt or would we like to re-implement the formatting specifically for the Nomad specification?

@danishprakash
Copy link
Contributor

@jrasell I'm not entirely sure but I guess this should involve using tfexec to invoke hclfmt internally and overwrite the source file? I've seen something similar happen over at terraform-ls for terraform fmt. If that sounds like the right direction, I'd like to work on this.

@mikenomitch
Copy link
Contributor

Hey @rounakdatta, yeah I think just using hclfmt would be fine!

If you base the implementation off of Packer's code, that could be a really good starting point if you'd like to contribute.

@danishprakash, I think anything that relies on TF is going to be a no-go from our perspective. While we of course love TF, we don't want that as a hard dependency for Nomad.

@danishprakash
Copy link
Contributor

@mikenomitch you're right, I think I thought we could do something similar to how we do it in terraform-ls for the formatting even from the client but I now realize, as the name suggests, tfexec is in fact built for executing terraform commands. The link you shared from Packer looks more aligned and reasonable. I'd like to give this a shot if no one else has started working on this already. Thanks

@rounakdatta
Copy link

Alright @mikenomitch , I've started looking into Packer's implementation. It's clear, and I think I'll be able to make some progress tonight/tomorrow. @danishprakash I was willing to try my first Nomad contribution through this issue 🙂

@danishprakash
Copy link
Contributor

Sure @rounakdatta, please feel free to take this up!

@sharmaansh21
Copy link

@rounakdatta Are you working on this ?

@shantanugadgil
Copy link
Contributor

The hclfmt used to trip up if you have inline bash code for initializing default variable. Does it treat the code properly in the newest version?

I have started using the file() function more and more to avoid inline shell code.

example:

      template = <<EOF
#!/bin/bash

MY_VAR=${MY_VAR:-"0"}
...
...
EOF

}

@Trojan295
Copy link
Contributor

Trojan295 commented Oct 3, 2022

Hello, if there is nobody working on that right now, I would like to implement the nomad fmt.

My idea is to implement it like here in hclfmt.

Do we have any requirements how the input should be passed?
My proposition it to pass the filepaths to the files, which should be formatted as arguments to the fmt, for example:

# format files
nomad fmt ./agent.hcl
nomad fmt /etc/nomad.d/nomad.hcl
nomad fmt /etc/nomad.d/nomad.hcl ./agent.hcl

# show help
nomad fmt

@mikenomitch
Copy link
Contributor

mikenomitch commented Oct 3, 2022

Hey @Trojan295, thanks so much for taking this! This will make a lot of people happy.

I think if you try to roughly match terraform's fmt command that would be ideal.

So if you have no argument (just nomad fmt ) mean "the current directory", and then also read from STDIN with a single dash "-" argument that'd be great. To show help you would enter nomad fmt -h.

I don't think it is necessary to add the -diff flag on the initial PR.

EDIT: If there's further discussion to be had on this interface, lets have it on the PR. Should be cleaner than on this issue. :)

@tgross tgross moved this from Needs Roadmapping to Triaging in Nomad - Community Issues Triage Oct 4, 2022
@tgross tgross self-assigned this Oct 4, 2022
@tgross tgross added this to the 1.4.x milestone Oct 4, 2022
@tgross
Copy link
Member

tgross commented Oct 4, 2022

PR is open on this and likely to merge for an early Nomad 1.4.x. I'll track this issue until that's been merged. Thanks, all!

@github-actions
Copy link

github-actions bot commented Feb 4, 2023

I'm going to lock this issue because it has been closed for 120 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 Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue help-wanted We encourage community PRs for these issues! stage/accepted Confirmed, and intend to work on. No timeline committment though. theme/cli theme/hcl type/enhancement
Projects
Development

Successfully merging a pull request may close this issue.

9 participants