cvm: fix tag/field SkipImageValidation conflicts #97

lbajolet-hashicorp · 2023-02-24T17:10:23Z

The SkipImageValidation' field and its tag, skip_region_validation, was duplicated in two structures, TencentCloudImageConfig, and TencentCloudAccessConfig`.

This resulted in unexpected behaviour, as setting the attribute in a template will set the config for both substructures.

To prevent this, we rename one attribute in the image_config, so that it's clear which attribute does what.

nywilken

It looks like we need to do more than just rename. I left a suggestion that should get this ready to merge. Please let me know if you have any questions.

nywilken · 2023-03-01T20:54:27Z

builder/tencentcloud/cvm/image_config.go

@@ -32,7 +32,7 @@ type TencentCloudImageConfig struct {
 	// after your image created.
 	ImageShareAccounts []string `mapstructure:"image_share_accounts" required:"false"`
 	// Do not check region and zone when validate.
-	SkipValidation bool `mapstructure:"skip_region_validation" required:"false"`
+	ImageSkipValidation bool `mapstructure:"image_skip_region_validation" required:"false"`


I see what happened here. This skip_region_validation is essentially the same as the one in Access Config, as you see before the validRegion function is in Access config. This field was probably copied just to support the check. Instead of renaming I would suggest adding a new config type struct in access_config.go just for the SkipValidation field that can then be embedded into both TencentCloudImageConfig and TencentCloudAccessConfig

Something like

type TenencentCloudRegionValidation struct { // Do not check region and zone when validate. SkipValidation bool `mapstructure:"skip_region_validation" required:"false"` }

This will ensure no breaking change and will work as intended.

Unfortunately I don't think we can do what you suggest as it will still imply a duplicate argument unfortunately.
I ended-up creating a single boolean in the parent config, and propagate its value to private fields on the substructures that needed it.

Hmm.. this is interesting if this is the case I suspect other plugins that use this embedded pattern would have this issue. Just in case I'm recommending we do something like https://github.com/hashicorp/packer-plugin-amazon/blob/2efec668b60523b60c43307e4b4a17b1d6328237/builder/common/run_config.go#L77 which is then embedded into each of the builders https://github.com/hashicorp/packer-plugin-amazon/blob/2efec668b60523b60c43307e4b4a17b1d6328237/builder/ebs/builder.go#L37

With this approach I fear we'll experience the same issue since a single builder has substructures that use this attribute, so if we make a shared structure and embed it in both config structs, it'll still be a conflict, hence the approach I took to propagate the value from an upper level to the sub-structures for later validation.
We could make a Meta structure to define such attributes later, but I would think we still have to manually propagate the values in this case.

Ah I see - this would be embedded twice within the same config structure for the cvm builder. For whatever reason I thought this was a config being shared across to builders. Sorry for not looking at the builder config entirely. Yeah this makes sense to me.

nywilken · 2023-03-08T20:05:19Z

builder/tencentcloud/cvm/image_config.go

@@ -31,8 +31,7 @@ type TencentCloudImageConfig struct {
 	// accounts that will be shared to
 	// after your image created.
 	ImageShareAccounts []string `mapstructure:"image_share_accounts" required:"false"`
-	// Do not check region and zone when validate.
-	SkipValidation bool `mapstructure:"skip_region_validation" required:"false"`
+	skipValidation     bool     `mapstructure:"_"`


Mapstructure tag is not needed.

Suggested change

skipValidation bool `mapstructure:"_"`

skipValidation bool

Good catch! I've removed it from the config

nywilken · 2023-03-08T20:05:30Z

builder/tencentcloud/cvm/image_config.go

@@ -32,7 +32,7 @@ type TencentCloudImageConfig struct {
 	// after your image created.
 	ImageShareAccounts []string `mapstructure:"image_share_accounts" required:"false"`
 	// Do not check region and zone when validate.
-	SkipValidation bool `mapstructure:"skip_region_validation" required:"false"`
+	ImageSkipValidation bool `mapstructure:"image_skip_region_validation" required:"false"`


Ah I see - this would be embedded twice within the same config structure for the cvm builder. For whatever reason I thought this was a config being shared across to builders. Sorry for not looking at the builder config entirely. Yeah this makes sense to me.

nywilken

This looks good to me. One small change request but otherwise good to go.

nywilken · 2023-03-08T20:06:35Z

This doesn't seem like a breaking change any more. So I removed the label. Please add it back if I am wrong.

lbajolet-hashicorp · 2023-03-08T20:11:48Z

It won't be breaking for any client I'd think, but it may change the behaviour for HCL2 users maybe. With the configuration in its current state, if someone set skip_image_validation = true in a template, the region validation would be skipped for the access config, but not for the image config, now it will be for both.
JSON users already experience this behaviour, so for those users, this won't change anything.

nywilken · 2023-03-08T20:17:25Z

It won't be breaking for any client I'd think, but it may change the behaviour for HCL2 users maybe. With the configuration in its current state, if someone set skip_image_validation = true in a template, the region validation would be skipped for the access config, but not for the image config, now it will be for both. JSON users already experience this behaviour, so for those users, this won't change anything.

Seeing as there were no previous bugs reported here it may not have been an issue and folks thought it was happening. Not sure. Feel free to check with @likexian on how you want to release and communicate. Great work here.

lbajolet-hashicorp · 2023-03-16T17:29:47Z

Since it's been a week since the PR was approved, and no objections were heard, I propose we merge this today (or tomorrow). @likexian, thoughts? Since this fixes some conflicts, and since the SDK has been updated to enforce not having conflicts in fields/tags in the latest release, we can bump the version of the SDK for this plugin, and release a new version if this is alright for you?

likexian · 2023-03-17T01:13:08Z

Since it's been a week since the PR was approved, and no objections were heard, I propose we merge this today (or tomorrow). @likexian, thoughts? Since this fixes some conflicts, and since the SDK has been updated to enforce not having conflicts in fields/tags in the latest release, we can bump the version of the SDK for this plugin, and release a new version if this is alright for you?

@lbajolet-hashicorp Sorry for the delay.

The changes LGTM, but I am not sure about backward compatibility, If there is no breaks, just go ahead.

The `SkipImageValidation' field and its tag, `skip_region_validation`, was duplicated in two structures, `TencentCloudImageConfig`, and `TencentCloudAccessConfig`. This resulted in unexpected behaviour, as setting the attribute in a template will set the config for both substructures for JSON templates, while on HCL templates, only one would be settable. To fix this discrepancy, we mutualise the field as a config argument for the global configuration object, and propagate its value to the appropriate configuration structures. This way, region validation can be skipped for both the AccessConfig, and the ImageConfig, in all cases.

lbajolet-hashicorp added bug breaking-change labels Feb 24, 2023

lbajolet-hashicorp requested a review from a team as a code owner February 24, 2023 17:10

nywilken suggested changes Mar 1, 2023

View reviewed changes

lbajolet-hashicorp force-pushed the fix_duplicate_field_tag-skip_image_validation branch 2 times, most recently from efaa6ea to ddd02d9 Compare March 8, 2023 17:16

nywilken reviewed Mar 8, 2023

View reviewed changes

nywilken approved these changes Mar 8, 2023

View reviewed changes

nywilken removed the breaking-change label Mar 8, 2023

lbajolet-hashicorp force-pushed the fix_duplicate_field_tag-skip_image_validation branch from ddd02d9 to 51d712c Compare March 8, 2023 20:13

lbajolet-hashicorp force-pushed the fix_duplicate_field_tag-skip_image_validation branch from 51d712c to e9eb5ac Compare March 8, 2023 20:20

lbajolet-hashicorp and others added 2 commits June 16, 2023 14:02

Bump github.com/hashicorp/packer-plugin-sdk@v0.4.0

fc9a7db

nywilken force-pushed the fix_duplicate_field_tag-skip_image_validation branch from e9eb5ac to fc9a7db Compare June 16, 2023 18:03

nywilken mentioned this pull request Jun 16, 2023

Bump github.com/hashicorp/packer-plugin-sdk from 0.3.4 to 0.4.0 #98

Closed

nywilken merged commit a7c5451 into main Jun 16, 2023

nywilken deleted the fix_duplicate_field_tag-skip_image_validation branch June 16, 2023 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cvm: fix tag/field SkipImageValidation conflicts #97

cvm: fix tag/field SkipImageValidation conflicts #97

Uh oh!

lbajolet-hashicorp commented Feb 24, 2023

Uh oh!

nywilken left a comment

Uh oh!

nywilken Mar 1, 2023

Uh oh!

lbajolet-hashicorp Mar 8, 2023

Uh oh!

nywilken Mar 8, 2023

Uh oh!

lbajolet-hashicorp Mar 8, 2023

Uh oh!

nywilken Mar 8, 2023

Uh oh!

nywilken Mar 8, 2023

Uh oh!

lbajolet-hashicorp Mar 8, 2023

Uh oh!

nywilken Mar 8, 2023

Uh oh!

nywilken left a comment

Uh oh!

nywilken commented Mar 8, 2023

Uh oh!

lbajolet-hashicorp commented Mar 8, 2023

Uh oh!

nywilken commented Mar 8, 2023

Uh oh!

lbajolet-hashicorp commented Mar 16, 2023

Uh oh!

likexian commented Mar 17, 2023

Uh oh!

Uh oh!

cvm: fix tag/field SkipImageValidation conflicts #97

cvm: fix tag/field SkipImageValidation conflicts #97

Uh oh!

Conversation

lbajolet-hashicorp commented Feb 24, 2023

Uh oh!

nywilken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nywilken left a comment

Choose a reason for hiding this comment

Uh oh!

nywilken commented Mar 8, 2023

Uh oh!

lbajolet-hashicorp commented Mar 8, 2023

Uh oh!

nywilken commented Mar 8, 2023

Uh oh!

lbajolet-hashicorp commented Mar 16, 2023

Uh oh!

likexian commented Mar 17, 2023

Uh oh!

Uh oh!