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: packer validate unsupported type error #13245

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gstvcruz
Copy link

packer validate would output the same error message four times per unsupported root block type found in a template (e.g., 'src' instead of 'source'). This behavior was due to a function being called four times for each file on each stage of the parsing.

Closes #13241

`packer validate` would output the same error message four times per
unsupported root block type found in a template (e.g., 'src' instead of
'source'). This behavior was due to a function being called four times
for each file on each stage of the parsing.
@gstvcruz gstvcruz requested a review from a team as a code owner December 21, 2024 00:11
Copy link

hashicorp-cla-app bot commented Dec 21, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

Have you signed the CLA already but the status is still pending? Recheck it.

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hey @gstvcruz,

Thanks for the PR! I have to say though, this kinda conflicts with PR #12607, which has been stale for a while, but reworks how parsing works for HCL2 templates.
The approach is slightly different as the other PR processes files once, and within the files all blocks are processed in one phase, compared to the current approach which relies on the file being parsed for every type of top-level block we're looking for.

That said, your approach seems functional (I tested it quickly on a couple templates I had lying around), and is definitely less intrusive than the other, so I can envision a path where yours is merged first in order to silence those extra error diags, then we can focus our efforts on the other PR to refactor how parsing is processed.

I don't necessarily advocate for one over the other, and while biased as I submitted the other one, I think we should discuss which one we should merge eventually, or both even maybe.

We'll discuss this in January since most of us are out of office for the holidays now, in the meantime thanks again for reporting the issue and tackling it, much appreciated!

@gstvcruz
Copy link
Author

Hey @lbajolet-hashicorp, thanks for your reply!

I believe refactoring the parsing process would be a more effective solution since the additional error diags are just a side effect of the current implementation. By refactoring the parsing, this bug would be resolved entirely. That said, the changes I proposed could act as a temporary measure to address the issue until the parsing refactor is merged.

I’d like to start working on the parser refactoring. Does the PR you mentioned provide enough information for me to get started? Also, why has it gone stale? Could you provide some context, please?

Thanks for you time!

@lbajolet-hashicorp
Copy link
Contributor

Hi again @gstvcruz,

Regarding the PR, it got stale mostly because we got sidetracked working on other things, this wasn't a high-priority issue to fix, plus there were a couple of comments on that that I'm not sure were addressed completely, so it needs discussions and a solid rebase as there are conflicts at the moment.
I had written it in the first place and I'm quite familiar with that part of the code, so if you don't mind, I'll take care of rebasing it and getting it reviewed for a later release ideally.

I do agree with you though, your PR is definitely simpler and would fix the issue in the meantime, so as I mentioned, I am not opposed to merging it in time of our next release, I'll bring this up when people are back from holidays, and we can decide what to do with it then.

Thanks for the response!

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.

packer validate . outputs same error message multiple times for unsupported block types
2 participants