Skip to content

Conversation

allisonlarson
Copy link
Member

@allisonlarson allisonlarson commented Sep 11, 2025

Description
Currently, nomad-pack injects metadata about the pack into the json definition of the job and not the HCL. When a pack job is managed outside of nomad-pack, like in the UI, the job loses its reference to the pack and is unable to be managed with nomad-pack until the job is completely removed.

This change takes the rendered HCL and injects the metadata into the job/meta block, instead of just the json. It will merge with the defined meta attribute or block if it exists. In the case that it cannot successfully parse the HCL, the original job will be returned. If the job is has multiple meta attributes/blocks defined, it will merge with one and return the other untouched. This is invalid, but will be caught in the next step of the process, which is parsing the templates in nomad. The approach is to inject these values with as little change to the underlying job as possible, and allow any existing invalid HCL to still be passed through and caught in nomad during the parsing step.

Reminders

  • Add CHANGELOG.md entry
  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

@allisonlarson allisonlarson requested review from a team as code owners September 11, 2025 00:01
@allisonlarson allisonlarson marked this pull request as draft September 11, 2025 00:07
@allisonlarson allisonlarson marked this pull request as ready for review September 15, 2025 17:49
tgross
tgross previously approved these changes Sep 15, 2025
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.

Great work on this @allisonlarson! One minor question: what about the old code that was setting the meta attributes? That's redundant now so can we remove it?

Comment on lines +47 to +49
// the values with any existing meta block or attribute. If the parsing fails,
// or the HCL is invalid, the original job is returned unmodified so that the
// errors can be caught later during job parsing.
Copy link
Member

Choose a reason for hiding this comment

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

I like that we're giving up here so that we avoid having to reimplement all the logic the parser already has. 👍

@allisonlarson
Copy link
Member Author

@tgross Correct, it should be redundant! I had left it as-is in the case of a fallback, but it makes more sense to remove it completely.

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.

LGTM! :shipit:

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.

2 participants