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

feat: Add nomad fmt #14779

Merged
merged 5 commits into from
Oct 6, 2022
Merged

feat: Add nomad fmt #14779

merged 5 commits into from
Oct 6, 2022

Conversation

Trojan295
Copy link
Contributor

This PR adds the nomad fmt command, which can be used to format and check Nomad configuration and jobspec files.

It formats the files according to the HCL syntax using hashicorp/hcl. It does not validate, if the files are correct Nomad or Jobspec files.

Closes #11757

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 3, 2022

CLA assistant check
All committers have signed the CLA.

@Trojan295 Trojan295 changed the title feat: Add nomad fmt WIP: feat: Add nomad fmt Oct 4, 2022
@tgross tgross self-requested a review October 4, 2022 17:30
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @Trojan295! This is great!

I've left some comments that can tighten this up and make it more consistent with both terraform fmt and the rest of the Nomad code base. Two more things:

Thanks again!

command/fmt.go Outdated Show resolved Hide resolved
command/fmt.go Outdated Show resolved Hide resolved
command/fmt.go Outdated Show resolved Hide resolved
command/fmt.go Outdated Show resolved Hide resolved
command/fmt.go Outdated Show resolved Hide resolved
command/fmt.go Outdated Show resolved Hide resolved
command/fmt.go Outdated Show resolved Hide resolved
command/fmt.go Show resolved Hide resolved
command/fmt_test.go Outdated Show resolved Hide resolved
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Looking great @Trojan295! Just a few last items to fix up and we'll be able to land this one.

command/testdata/fmt/nomad.in.hcl Show resolved Hide resolved
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's fix the newlines at the end of these files.

website/content/docs/commands/fmt.mdx Outdated Show resolved Hide resolved
website/content/docs/commands/fmt.mdx Outdated Show resolved Hide resolved
website/content/docs/commands/fmt.mdx Show resolved Hide resolved
website/content/docs/commands/fmt.mdx Show resolved Hide resolved
website/content/docs/commands/fmt.mdx Show resolved Hide resolved
@tgross tgross added this to the 1.4.x milestone Oct 5, 2022
@tgross tgross self-assigned this Oct 5, 2022
@Trojan295
Copy link
Contributor Author

Thanks @tgross for the review! I will update this PR tomorrow. :)

@Trojan295
Copy link
Contributor Author

Trojan295 commented Oct 6, 2022

I pushed the changes.

Instead of putting the testdata files as const into the Go source, I added to ignore the ./command/testdata by make hclfmt.
I kinda like it more to keep them separate, but if you prefer to include this in the Go sources, I can change this also.

@tgross tgross force-pushed the feat/add-nomad-fmt branch from a89fcbd to b4eada2 Compare October 6, 2022 20:40
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Looks great, @Trojan295! Thanks for sticking with it, I know PRs for new commands can feel like a whole lot of nitpicking 😀

CircleCI seems to have gotten confused about the PR and can't run the build. So I just rebased this branch on main to get that moving again. Once that finishes, we can get this merged. (I'll probably have to do that tomorrow morning, as I'm close to end-of-day here.) Done

@tgross
Copy link
Member

tgross commented Oct 6, 2022

@Trojan295 this has been merged and a backport bot will merge it to the 1.4.x release branch for the next patch release of Nomad . If that bot can't figure things out and requests a review from you, don't worry about it and I'll fix it in the morning; it's one of those fiddly process things we try not to expose to contributors but the bot is working thru a bit of a queue at the moment and I won't be there to immediately catch it. 😀

@Trojan295
Copy link
Contributor Author

Looks great, @Trojan295! Thanks for sticking with it, I know PRs for new commands can feel like a whole lot of nitpicking 😀

Wasn't so bad :). I just had a though, that it would be nice, if some of those docs/CLI help could be generated. E.g. we have the help in mdx and this gets automatically added to the website and also the CLI help is generated out of it.

@tgross
Copy link
Member

tgross commented Oct 7, 2022

I just had a though, that it would be nice, if some of those docs/CLI help could be generated.

Yes! I know a few folks have been tinkering around with better CLI generation internally for that reason.

@github-actions
Copy link

github-actions bot commented Feb 5, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, 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 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/1.4.x backport to 1.4.x release line theme/cli
Projects
Development

Successfully merging this pull request may close these issues.

Please add nomad fmt command
3 participants