Remove build flag and re-introduce provision method attribute #122
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The build flag was introduced as the replacement for the "method" attribute. Earlier, the method could be set to "build" or "image", which is then used by Foreman. The initial creation of a host also needed the build flag to be set in the API request to signal Foreman that it should create the machine. This lead to the problem that Terraform configs containing "build = true" would reset the build flag in Foreman even for existing, already built machines, resetting them at the next boot. This is dangerous and should be avoided. The commit removes the build flag completely from Terraform and uses the "provision_method" attribute from the Foreman API (again). We will have to find an additional attribute to support the use case of "enforced rebuild" that originally lead to the switch. The "managed" flag is not changed. Refs terraform-coop#40 Refs terraform-coop#68 Refs terraform-coop#106
agriffit79
reviewed
May 19, 2023
…ning See examples/host/vmware_imagebased_host.tf for a VMware-backed image VM Refs terraform-coop#103 Refs terraform-coop#102 Refs terraform-coop#105
|
My tests with self-compiled binary were successful either with network-based or image-based provisioning method (running against foreman v3.6.1) My test code (image-based provisioning example with vSphere provider) |
bitkeks
commented
Jun 27, 2023
| @@ -953,6 +946,10 @@ func setResourceDataFromForemanHost(d *schema.ResourceData, fh *api.ForemanHost) | |||
| log.Printf("[WARN] error setting compute attributes: %s", err) | |||
| } | |||
|
|
|||
| // See issue #115 for "Build" attribute | |||
| d.Set("managed", fh.Managed) | |||
bitkeks
commented
Jun 28, 2023
foreman/resource_foreman_host.go
Outdated
| // log.Debugf("computeResource: [%+v]", computeResource) | ||
|
|
||
| var diags diag.Diagnostics | ||
| if h.ProvisionMethod == "image" { // && strings.ToLower(computeResource.Provider) == "vmware" { |
There was a problem hiding this comment.
This might differ in other providers, so a hard fail when image_id is not given might introduce problems for other users. See #125 (reply in thread)
Removes an experimental check of the compute_attributes when using "image"-based host creation. The backend providers need different values. VMware for example needs the image_id as UUID, see the example file in examples/host/vmware_imagebased_host.tf Refs terraform-coop#125
agriffit79
approved these changes
Jun 30, 2023
foreman/resource_foreman_host.go
Outdated
| diags = append(diags, diag) | ||
| return diags | ||
|
|
||
| } else if _, ok := h.ComputeAttributes["image_id"]; !ok { |
There was a problem hiding this comment.
image_id might be required for vmware but I don't believe it is for other plugins. It can also be implicitly provided through a compute_profile. As in this example. Every virtualisation plugin is different, I think trying to validate the content of compute_attributes is going to be impossible. Every PR risks breaking existing behaviour as no one can test all possible plugins.
bitkeks
added a commit
to mindfulsecurity/terraform-provider-foreman
that referenced
this pull request
Jul 17, 2023
PR terraform-coop#122 removed the "build" argument, removing the possibility to enforce setting the Foreman-internal build flag to "true". This commit re-introduces this ability, but with a more explicit argument name: "set_build_flag". And it defaults to "false", so that not specifying it does not toggle the flag to "true" on accident. Resolves terraform-coop#130 See terraform-coop#125
bitkeks
added a commit
that referenced
this pull request
Jul 19, 2023
PR #122 removed the "build" argument, removing the possibility to enforce setting the Foreman-internal build flag to "true". This commit re-introduces this ability, but with a more explicit argument name: "set_build_flag". And it defaults to "false", so that not specifying it does not toggle the flag to "true" on accident. Resolves #130 See #125
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The build flag was introduced as the replacement for the "method" attribute. Earlier, the method could be set to "build" or "image", which is then used by Foreman. The initial creation of a host also needed the build flag to be set in the API request to signal Foreman that it should create the machine.
This lead to the problem that Terraform configs containing "build = true" would reset the build flag in Foreman even for existing, already built machines, resetting them at the next boot. This is dangerous and should be avoided.
The commit removes the build flag completely from Terraform and uses the "provision_method" attribute from the Foreman API (again).
We will have to find an additional attribute to support the use case of "enforced rebuild" that originally lead to the switch.
The "managed" flag is not changed.
Refs #40
Refs #68
Refs #106