-
-
Notifications
You must be signed in to change notification settings - Fork 971
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
Allow hcl format from stdin #3288
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Left some minor notes.
Co-authored-by: Yousif Akbar <11247449+yhakbar@users.noreply.github.com>
…runt into feature/hclfmt-stdin-2926
Hi, looks like code failed on linter:
|
@denis256 thank you for feedback! Fixed it! |
Hey @alikhil We are still getting errors in CI:
Are you able to run that test locally? |
Quality Gate passedIssues Measures |
It seems like test was failing in main branch when I forked repo.
|
@yhakbar hello! Does it still fails on tests? |
Hey @alikhil , I just re-ran the test, and this is failing:
Are you able to run that test locally? You should be able to with |
Hi @yhakbar Yes, when I run the test locally it passes:
|
Hey @alikhil , I tried to kick off CI for the changes here, and ran into errors from our linter. Could you please run
The test does pass for me locally as well, so I suspect that once the code passes linting, we should be able to iron out any remaining issues pretty quickly. We've left this PR open for quite a while, so please do the following so that we can make sure this PR gets merged ASAP:
If you receive more feedback that requires changes, please continue to iterate like that. Don't hesitate to remind us to review if we're slow to respond. |
Hi @yhakbar ! I think I fixed original linting issue, but in my dev enviroment (windows) I can not make linter green because of other issues not related to my PR:
|
Same error in make run-lint:
But initial error |
Hello,
CICD uses different version - |
@denis256 hello! Sure! Output fov 1.59.1 is the same:
|
|
@denis256 could you recheck please? |
I added a couple of notes, PR looks outdated from |
@denis256 thank you for feedback. I have updated PR from |
FlagNameTerragruntHCLFmt = "terragrunt-hclfmt-file" | ||
FlagNameTerragruntCheck = "terragrunt-check" | ||
FlagNameTerragruntDiff = "terragrunt-diff" | ||
FlagNameTerragruntHCLFmtStdin = "terragrunt-hclfmt-stdin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing integration test to validate end-to-end usage of --terragrunt-hclfmt-stdin
flag, for example I commented lines 37-42 and all tests pass...
Examples:
https://github.com/gruntwork-io/terragrunt/blob/main/test/integration_download_test.go#L260
https://github.com/gruntwork-io/terragrunt/blob/main/test/integration_test.go
Hey @alikhil , Thanks again for opening this PR. I'm going to move this to in-draft, as it's gone stale. Please request a re-review when ready! |
Resolves #2926
Description
Fixes #2926 .
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added
--terragrunt-hclfmt-stdin
flag for hclfmt commandMigration Guide