-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Expose versionFingerprint instead of versionId from packer template #12803
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.
Thanks for pushing this @devashish-patel,
The base is here, I left a few comments on the code/docs, we can iterate on this and get it merged soon.
hcl2template/types.packer_config.go
Outdated
@@ -122,12 +124,22 @@ func (cfg *PackerConfig) EvalContext(ctx BlockContext, variables map[string]cty. | |||
|
|||
iterID, ok := cfg.HCPVars["iterationID"] | |||
if ok { | |||
log.Printf("[WARN] Deprecation: Contextual Variable `iterationID` has been deprecated packer context. " + |
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.
The log here will be problematic, the context is used for evaluating HCL expressions, and we create it often.
The fact that iterationID
exists does not mean it is used, that happens in other places.
Note: it may not be trivial to figure out with certainty that it's used, as evaluation is done by the HCL library, outside of Packer's code.
hcl2template/types.packer_config.go
Outdated
ectx.Variables[packerAccessor] = cty.ObjectVal(map[string]cty.Value{ | ||
"version": cty.StringVal(cfg.CorePackerVersionString), | ||
"iterationID": iterID, | ||
}) | ||
} | ||
|
||
versionFingerprint, ok := cfg.HCPVars["versionFingerprint"] |
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.
That will make iterationID
unusable in this state unfortunately, since we create the object in this branch, and we do the same above, so this assignation will erase the previous value.
I would suggest changing a couple of things here, the previous approach is probably not valid anymore.
Let's remove the creation of the packerAccessor
object from the declaration of the context, we can build it along the way, and only create it when it is in its final state.
packerVars := map[string]cty.Value{
"version": cty.StringVal(cfg.CorePackerVersionString),
"iterationID": cty.UnknownVal(cty.String),
"versionFingerprint": cty.UnknownVal(cty.String),
}
iterID, ok := cfg.HCPVars["iterationID"]
if ok {
packerVars["iterationID"] = cty.StringVal(iterID)
}
versionFP, ok := cfg.HCPVars["versionFingerprint"]
if ok {
packerVars["versionFingerprint"] = cty.String(versionFP)
}
ectx.Variables[packerAccessor] = cty.ObjectVal(packerVars)
ami_name = "packer-example" | ||
// ... | ||
run_volume_tags = { | ||
hcp_version_fingerprint = packer.versionFingerprint |
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.
I wonder, does that work? I'm not certain the fingerprint will be instanciated by the time we evaluate this.
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.
To be clear if it doesn't we should fix it later, but I'm cautious giving this example if it doesn't work
} | ||
``` | ||
|
||
|
||
```shell-session | ||
==> vanilla.amazon-ebs.cannonical-ubuntu-server: Adding tags to source instance | ||
vanilla.amazon-ebs.cannonical-ubuntu-server: Adding tag: "Name": "Packer Builder" |
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.
We should update the logs here so they don't show the hcp_iteration_id
in there right?
@@ -132,6 +132,8 @@ parenthesis may through off your shell escaping otherwise. | |||
|
|||
# HCP Packer Iteration ID | |||
|
|||
~> **Note**: Deprecation: Contextual Variable `iterationID` has been deprecated packer context. Please use `versionFingerprint` variable instead. |
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.
I'm not sure we can suggest using versionFingerprint
without encouraging them to update the data sources that use it at the same time.
I would maybe reword this here so it explains the rationale behind the change.
~> **Note**: Deprecation: Contextual Variable `iterationID` has been deprecated packer context. Please use `versionFingerprint` variable instead. | |
~> **Note**: the `packer.iterationID` variable is now deprecated and will be removed in a future version of Packer. HCP Packer Versions should be accessed with their fingerprint instead. The `packer.versionFingerprint` variable is now exposed to be used in its stead with the new HCP Packer data sources. |
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.
Left one final nit, but aside from that, we're good to go!
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
No description provided.