FR-5402 - ubuntu-image classic always passes `--vcs=auto` to germinate even when told not to #146

upils · 2023-09-27T12:34:06Z

Fixing https://bugs.launchpad.net/ubuntu-image/+bug/2021411

codecov · 2023-09-28T15:42:30Z

Codecov Report

Merging #146 (aa24b6f) into main (5b10b91) will increase coverage by 2.40%.
Report is 9 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head aa24b6f differs from pull request most recent head 6f8e4be. Consider uploading reports for the commit 6f8e4be to get more accurate results

@@            Coverage Diff             @@
##             main     #146      +/-   ##
==========================================
+ Coverage   87.32%   89.72%   +2.40%     
==========================================
  Files          11       11              
  Lines        2437     2589     +152     
==========================================
+ Hits         2128     2323     +195     
+ Misses        289      244      -45     
- Partials       20       22       +2

Files	Coverage Δ
internal/helper/helper.go	`23.10% <100.00%> (+18.59%)`	⬆️
internal/imagedefinition/image_definition.go	`100.00% <ø> (ø)`
internal/statemachine/helper.go	`98.67% <100.00%> (ø)`

... and 3 files with indirect coverage changes

sil2100

Sadly, I know you already did work for this fix here, but I will have to indeed object to the renaming of vcs to no-vcs. One thing is what you mentioned in the LP bug already: backwards compatibility. I wouldn't want us to fix something by changing the schema per-se. Second, the schema so far doesn't really have any 'negative keys', aka. this would be the first inverse field in the yaml file. And I don't think it's very intuitive in this case.

It is indeed a troublesome feature of the boolean in this case, and I think it's a shame that the schema handling does not really handle fields being unassigned in some special way (like via a separate flag somewhere). Potentially this problem can appear for any boolean value, and I don't think we should build out schema against limitations of the language/framework we're using.

I know you mentioned a pointer approach, which I guess could be doable - we could design it in such a way to use it in any other future booleans if the need appears. I was personally even thinking of using a string here, but that seems a bit worse? Anyway, could you take a look at it so that we don't have to change the schema? Thank you!

…om overriding false value.

upils · 2023-10-03T07:58:02Z

Fixed it using pointers to booleans. I also updated the definition of KeepEnabled even though it is not used. I understand this is to keep a kind of compatibility with *crafts configuration, but should we remove it?

sil2100

This looks good. And I like the additional improvements to the testing story. This feels good for merging.

As for KeepEnabled - this is a bug/missing-feature if it's not used. I thought we did handle it properly. Might be that it got missed due to deadlines. Basically it means whether the enabled PPA should be kept enabled on the built system after package installation. Let me create a card for it.

sil2100 · 2023-10-03T09:29:51Z

internal/helper/helper.go

+				case reflect.Slice:
+					defaultValues := strings.Split(defaultValue, ",")
+					field.Set(reflect.ValueOf(defaultValues))
+				case reflect.Bool:


I'm thinking if maybe at this point we shouldn't just drop this part. Maybe it's better to remind us that it's not really 'safe' to use default values for pure booleans. Sure, there are cases where this would work, but it's unreliable IMO. So maybe I'd propose to remove this line and let us error out when trying to set a default. We should probably use pointers-to-booleans for every boolean with a default value.

I agree that is unreliable. I will error out when trying to set a default on a bool. I will also add a specific test case on the image definition to make sure we never add a bool with a default. Without this test we risk letting it pass and users will face an error they cannot do anything about.

sil2100 · 2023-10-03T12:03:31Z

internal/helper/helper_test.go

+	A []*S4
+}
+
+func TestSetDefaults(t *testing.T) {


sil2100 · 2023-10-03T12:04:15Z

internal/imagedefinition/image_definition.go

@@ -97,7 +97,7 @@ type PPA struct {
 	PPAName     string `yaml:"name"         json:"PPAName"               jsonschema:"pattern=^[a-zA-Z0-9_.+-]+/[a-zA-Z0-9_.+-]+$"`
 	Auth        string `yaml:"auth"         json:"Auth,omitempty"        jsonschema:"pattern=^[a-zA-Z0-9_.+-]+:[a-zA-Z0-9]+$"`
 	Fingerprint string `yaml:"fingerprint"  json:"Fingerprint,omitempty"`
-	KeepEnabled bool   `yaml:"keep-enabled" json:"KeepEnabled"           default:"true"`
+	KeepEnabled *bool  `yaml:"keep-enabled" json:"KeepEnabled"           default:"true"`


As for this, I'll mention more about it in my main review comment, but I missed that it's not used! I was sure we used it already!

sil2100 · 2023-10-03T12:07:40Z

Ok, before merging, could you maybe remove the boolean handling though?

sil2100

Excellent!

sil2100 · 2023-10-04T07:58:06Z

hm, linting failed, but that doesn't seem to be a real issue?

upils · 2023-10-04T08:02:46Z

Yeah, just a minor linting error about import order. This is fixed.

Improve and add tests to spot issue with germinate flag

d3dfbbb

upils marked this pull request as draft September 27, 2023 12:37

Fully cover SetDefaults except boolean handling

3fcb934

upils requested a review from sil2100 September 29, 2023 07:09

upils marked this pull request as ready for review September 29, 2023 07:10

sil2100 suggested changes Oct 2, 2023

View reviewed changes

Transform bool in image definition to ptr to bool to avoid default fr…

f1be00d

…om overriding false value.

upils force-pushed the vcs-option-broken branch from 2c7625c to f1be00d Compare October 3, 2023 07:55

upils requested a review from sil2100 October 3, 2023 08:55

sil2100 approved these changes Oct 3, 2023

View reviewed changes

upils requested a review from sil2100 October 3, 2023 12:55

sil2100 approved these changes Oct 4, 2023

View reviewed changes

Reject setting default on boolean

6f8e4be

upils force-pushed the vcs-option-broken branch from aa24b6f to 6f8e4be Compare October 4, 2023 08:02

sil2100 merged commit 6193013 into main Oct 4, 2023

upils deleted the vcs-option-broken branch November 28, 2023 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FR-5402 - ubuntu-image classic always passes `--vcs=auto` to germinate even when told not to #146

FR-5402 - ubuntu-image classic always passes `--vcs=auto` to germinate even when told not to #146

upils commented Sep 27, 2023 •

edited

Loading

codecov bot commented Sep 28, 2023 •

edited

Loading

sil2100 left a comment

upils commented Oct 3, 2023 •

edited

Loading

sil2100 left a comment

sil2100 Oct 3, 2023

upils Oct 3, 2023

sil2100 Oct 3, 2023

sil2100 Oct 3, 2023

sil2100 commented Oct 3, 2023

sil2100 left a comment

sil2100 commented Oct 4, 2023

upils commented Oct 4, 2023

FR-5402 - ubuntu-image classic always passes --vcs=auto to germinate even when told not to #146

FR-5402 - ubuntu-image classic always passes --vcs=auto to germinate even when told not to #146

Conversation

upils commented Sep 27, 2023 • edited Loading

codecov bot commented Sep 28, 2023 • edited Loading

Codecov Report

sil2100 left a comment

Choose a reason for hiding this comment

upils commented Oct 3, 2023 • edited Loading

sil2100 left a comment

Choose a reason for hiding this comment

sil2100 Oct 3, 2023

Choose a reason for hiding this comment

upils Oct 3, 2023

Choose a reason for hiding this comment

sil2100 Oct 3, 2023

Choose a reason for hiding this comment

sil2100 Oct 3, 2023

Choose a reason for hiding this comment

sil2100 commented Oct 3, 2023

sil2100 left a comment

Choose a reason for hiding this comment

sil2100 commented Oct 4, 2023

upils commented Oct 4, 2023

FR-5402 - ubuntu-image classic always passes `--vcs=auto` to germinate even when told not to #146

FR-5402 - ubuntu-image classic always passes `--vcs=auto` to germinate even when told not to #146

upils commented Sep 27, 2023 •

edited

Loading

codecov bot commented Sep 28, 2023 •

edited

Loading

upils commented Oct 3, 2023 •

edited

Loading