[feature] Nested Pack updates #403
Conversation
angrycub
added
theme/core
type/enhancement
theme/template/function
There was a problem hiding this comment.
This is amazing work @angrycub!
There were a few empty files and wondered if we can get rid of these?
In a number of places we use fmt.Errorf but don't format the error, so it would be nice to use errors.New instead.
I think some files are missing the license header.
It might be nice to have smaller unit tests alongside the bigger ones. I noticed a few functions did not have direct tests, which can sometimes be useful.
_self is used a bit and while I noted it in the code, I thought it was worth asking whether this should be a "top level" const/type somewhere?
The work to support v1/v2 parsers is huge and I think the rationale you wrote makes sense, however, I do wonder how this compares to the maintainability overhead moving forward. If this is a stop-gap while packs move over to the new style, then I think we should call this out in a number of places, and plan to remove it. Alternatively, could old style pack users just use the older releases, so it creates a clear cut?
| return out | ||
| } | ||
|
|
||
| // NOTE: Beyond here, things get weird. |
There was a problem hiding this comment.
Is it worth expanding on why it's weird?
There was a problem hiding this comment.
From discussions with @angrycub I gather it would just be very very complicated explaining this in a comment :) Still, probably worth it.
There was a problem hiding this comment.
Resolved in 1f0cad2
angrycub
force-pushed
the
f-nested-var-deps
branch
2 times, most recently
from
ad8c31d
to
7e1cef3
Compare
- Remove cruft - Remove unused `NewWithPrefix` function - Switch to pointer receiver - Unshadow `variables` package - Remove unnecessary true: I was using an initial comparison to `true` so that all of the other comparisons ended up in a straight line down, but it's not idiomatic. - Remove unused argument: I think the argument predated the accumulator - Remove unused `DiagsFileNotFound`
Remove an extra layer of funcs that don't need to be there.
- Remove boilerplate from generated files - Add missing final linefeeds
Since the current pack's template context key is a magic value, make it a constant.
Aggregate conversion implementations into the SDK rather than inside the Pack implementation code.
Deals with `any` when explicitly set
Renames - PackID to pack.ID - VariableID to variables.ID Moves - varfile.Overrides to sdk/variables.Overrides
angrycub
force-pushed
the
f-nested-var-deps
branch
from
aba598c
to
28ecbfe
Compare
Mark helper funcs as t.Helper()
There was a problem hiding this comment.
Monumental work, @angrycub! I left a few comments that I'd like to see addressed before merging, but I am approving, I think in its current shape the PR looks good to be merged.
Once again, the scope of improvements is truly amazing, congrats! 🎉
internal/pkg/varfile/varfile_test.go
Outdated
| must.MapNotContainsKey(t, d2.Overrides, "p1") | ||
| }) | ||
|
|
||
| //TODO: Investigate this broken test. |
There was a problem hiding this comment.
Is this something we should still investigate?
| return out | ||
| } | ||
|
|
||
| // NOTE: Beyond here, things get weird. |
There was a problem hiding this comment.
From discussions with @angrycub I gather it would just be very very complicated explaining this in a comment :) Still, probably worth it.
Update all receivers except Error() to be pointer receivers.
Try to make some of the inner magic more clear
Move PackIDKeyedVarMap to variables; remove unused funcs.
Improved Nested Pack Support
Abstract
To better support nested pack dependencies, this PR takes a different approach to the way in which dependency packs are stored within one another and how the template context is organized. It does, however, retain the ability to process un-updated packs using a "--parser-v1" flag.
New variable naming scheme
When supplying variables to the pack, the names must be provided using fully-qualified dotted references. For example, you would use the following command to specify the value of variable "dcs" on a file-system based, root pack named "app" using the
--varflagEnvironment Variables
For envvars this is more tricky, dots are problematic, but almost any substitution you could make would yield an ambiguous name to parse. For now, variables can be read from the environment as
NOMAD_PACK_VAR_app.dcsand have to be provided by a non-shell wrapper likeenvor custom tooling. There will be future work to try and iron out this particular wrinkle.Implementation notes
There is a lot of magic that happens in the variable override file parsing to allow for dotted left-hand side variables in the HCL. It leverages a cheat based on how HCL deals with map keys versus attributes and wraps the filedata in map syntax. Any produced hcl.Diagnostics based on that data have to be touched up before being presented to the end user. This is "clever" code, where clever in this case is probably a dirty word.
Supporting the new and current syntax
One of the other feature(esque) components of this PR is to provide some time for adopters to start migrating their packs to the new syntax by continuing to support the current syntax. I've been told to rip the bandaid off, and I can appreciate that from a code complexity standpoint. However, it made me feel better about unleashing a new template context shape and supportive functions knowing that I was less likely to leave someone fully broken if we could support the current syntax.
template_context.go
Currently, there is a non-trivial amount of entanglement in the template_context.go that I don't love, and the parser/renderer abstraction is super leaky there. But since it's current state is transitory and we wanted to move this along, it's grody but functioning.
funcMaps
In supporting both versions of pack syntax, I wanted to try to give Pack authors and consumers as much feedback for when (not if) something gets crossed up between the syntaxes. When using the v1 parser, the funcmap is built with functions having the same name as the v2 parser that return meaningful runtime errors to the end user.
PackTemplateError
This is an attempt to help users who run a pack having v1 syntax using the v2 parser. Because the contexts are completely differently shaped, the most likely error that will happen is a nil pointer exception that leaks the PackContextable detail during template rendering. Go's text/template errors are certainly not designed to be end-user facing and PackContextable is an internal implementation detail, so the plan here is to make more user friendly errors starting with template execution errors. There are opportunities to grow this out over time if it's desirable.
Refactoring
This is the part of this PR that profoundly bloats it.
Typed map keys
While trying to reorganize the template context, it became nearly impossible to track exactly what was going on in the
map[string]anyand map[string]map[string]any` that I wanted to come back and move to typed map keys. This change was comically sprawly.Test fixture rewrites and reorganization
Since there are now two syntaxes to parse, I brought back the tests for the earlier syntax. I reorganized the fixtures so that when the time comes, the v1 fixtures should be easily removed.
Several tests were printing out the test case name in their error rather than taking advantage of
t.Runto build subtests, so many test cases were updated to switch to subtests.Rehoming some errors and HCL diags
There were some opportunities to move some of the errors and hcl.Diagnostics to the common errors package.