[core]: allow empty struct member updates by kayrus · Pull Request #3190 · gophercloud/gophercloud

kayrus · 2024-09-27T08:01:53Z

coveralls · 2024-09-27T08:07:49Z

coverage: 78.726%. remained the same
when pulling c9703e7 on kayrus:flavor-pointer
into 8caccbb on gophercloud:master.

EmilienM · 2024-10-04T02:14:12Z

I've approved it but I need an extra pair of eyes on this one, ping @pierreprinetti

pierreprinetti

I understand the value of changing all types to pointer types in UpdateOpts structs while keeping the omitempty tag, to make a clear difference between "I want to change this property" and "I don't want to change this property", including the case where the required new value happens to be the zero value for the type (and as a consequence wasn't sent to the server before this patch).

This technically is a superfluous change in some instances. For example, let's take the servers package. It's useless to use *string in Name, because the API refuses the empty string as a name. Same goes for accessIPv4 and accessIPv6. However, I agree with you that this change brings consistency and consistency is valuable.

My comments propose the addition of a convenience generic function that given a value, returns a pointer to it. It may make code more easily readable and reduce the LOC count of this patch. Please consider adding it in an internal package (I propose internal/ptr) to use it in both unit and acceptance tests.

pierreprinetti · 2024-10-04T09:26:35Z

internal/acceptance/openstack/loadbalancer/v2/flavorprofiles_test.go

+	name := tools.RandomString("TESTACCT-", 8)
 	flavorProfileUpdateOpts := flavorprofiles.UpdateOpts{
-		Name: tools.RandomString("TESTACCTUP-", 8),
+		Name: &name,


Suggested change

Name: &name,

Name: ptr.To(tools.RandomString("TESTACCTUP-", 8)),

pierreprinetti · 2024-10-04T09:31:33Z

internal/acceptance/openstack/loadbalancer/v2/loadbalancer.go

 		Description:     description,
 		FlavorProfileId: flavorProfile.ID,
-		Enabled:         true,
+		Enabled:         new(bool),


Suggested change

Enabled: new(bool),

Enabled: ptr.To(true),

pierreprinetti · 2024-10-04T09:34:27Z

openstack/compute/v2/servers/testing/requests_test.go

+	name := "new-name"
+	actual, err := servers.Update(context.TODO(), client, "1234asdf", servers.UpdateOpts{Name: &name}).Extract()


Suggested change

name := "new-name"

actual, err := servers.Update(context.TODO(), client, "1234asdf", servers.UpdateOpts{Name: &name}).Extract()

actual, err := servers.Update(context.TODO(), client, "1234asdf", servers.UpdateOpts{Name: ptr.To("new-name")}).Extract()

pierreprinetti · 2024-10-04T09:44:06Z

internal/acceptance/openstack/compute/v2/aggregates_test.go

+		Name:             &name,
+		AvailabilityZone: &availabilityZone,


Please consider adding an internal convenience function to indirect values on-the-fly:

diff --git a/internal/ptr/to.go b/internal/ptr/to.go new file mode 100644 index 00000000..0f2fd60f --- /dev/null +++ b/internal/ptr/to.go @@ -0,0 +1,3 @@ +package ptr + +func To[T any](v T) *T { return &v }

After that, this can become:

Suggested change

Name: &name,

AvailabilityZone: &availabilityZone,

Name: ptr.To("new_aggregate_name"),

AvailabilityZone: ptr.To("new_azone"),

As a byproduct, it's a function we can point users to in case they want to copy-paste.

why internal? this can be reused in other packages. also it would be helpful to have a helper to get a pointer value on-the-fly. and if it's a nil, return an empty struct.

Internal so that it doesn't increase the package's API. I don't think we want that function to be part of the exported Gophercloud funtions.

it would be helpful to have a helper to get a pointer value on-the-fly. and if it's a nil, return an empty struct

That looks complex. Do you envision that as another convenience function for testing? Would you use that in this PR?

pierreprinetti · 2024-10-04T09:45:19Z

internal/acceptance/openstack/compute/v2/secgroup_test.go

 	updateOpts := secgroups.UpdateOpts{
-		Name:        newName,
+		Name:        &newName,
 		Description: &description,


This can become:

Suggested change

Description: &description,

Description: ptr.To(""),

I'll keep this as is, since description is used below in code.

pierreprinetti · 2024-10-04T11:14:43Z

openstack/loadbalancer/v2/flavors/testing/fixtures.go

 				"name": "Basic",
 				"description": "A basic standalone Octavia load balancer.",
-				"enabled": true,
+				"enabled": false,


This can be reverted once you apply my suggestion above

no, we want to ensure that this can be set to false.

OK; but then please add a new test to verify the new functionality, and leave this old test here doing its job of testing regressions :)

pierreprinetti · 2024-10-04T11:15:18Z

openstack/loadbalancer/v2/flavors/testing/requests_test.go

 		Name:            "Basic",
 		Description:     "A basic standalone Octavia load balancer.",
-		Enabled:         true,
+		Enabled:         new(bool),


Suggested change

Enabled: new(bool),

Enabled: ptr.To(true),

pierreprinetti · 2024-10-04T11:16:02Z

openstack/loadbalancer/v2/flavors/testing/requests_test.go

+		Name:        &name,
+		Description: &description,
+		Enabled:     &enabled,


consider using ptr.To

pierreprinetti · 2024-10-11T15:12:30Z

internal/ptr/ptr.go

+func From[T any](p *T) T {
+	if p == nil {
+		var v T
+		return v
+	}
+	return *p
+}


This doesn't seem to be used anywhere. Please remove

Octavia Flavor's `enabled` is true by default

pierreprinetti

Thanks! This is for v3

kayrus requested a review from a team September 27, 2024 08:01

github-actions bot added edit:loadbalancer This PR updates loadbalancer code edit:compute This PR updates compute code semver:major Breaking change labels Sep 27, 2024

kayrus force-pushed the flavor-pointer branch from 7c84d45 to e34cfc6 Compare September 27, 2024 08:59

EmilienM previously approved these changes Oct 4, 2024

View reviewed changes

pierreprinetti added the hold Do not merge label Oct 4, 2024

pierreprinetti reviewed Oct 4, 2024

View reviewed changes

kayrus dismissed EmilienM’s stale review via a87cd22 October 9, 2024 19:14

kayrus force-pushed the flavor-pointer branch 4 times, most recently from a43d466 to 84deba4 Compare October 9, 2024 20:00

github-actions bot added the edit:actions This PR updates GitHub Actions code label Oct 9, 2024

kayrus force-pushed the flavor-pointer branch from 84deba4 to e89ce11 Compare October 9, 2024 20:01

pierreprinetti reviewed Oct 11, 2024

View reviewed changes

[core]: allow empty struct member updates

470f1bc

kayrus force-pushed the flavor-pointer branch from e89ce11 to 9a4af6a Compare October 14, 2024 13:44

[octavia]: allow creating a disabled flavor

c9703e7

Octavia Flavor's `enabled` is true by default

kayrus force-pushed the flavor-pointer branch from 9a4af6a to c9703e7 Compare October 14, 2024 14:23

pierreprinetti removed the hold Do not merge label Dec 6, 2024

pierreprinetti approved these changes Dec 6, 2024

View reviewed changes

pierreprinetti merged commit 69f3895 into gophercloud:master Dec 6, 2024

kayrus deleted the flavor-pointer branch May 23, 2025 13:56

mandre mentioned this pull request Sep 26, 2025

Flavor: add mutability for all fields that support it k-orc/openstack-resource-controller#514

Closed

mandre mentioned this pull request Sep 26, 2025

Some resources can't unset values in UpdateOpts #3520

Open

	Name: &name,
	Name: ptr.To(tools.RandomString("TESTACCTUP-", 8)),

		name := "new-name"
		actual, err := servers.Update(context.TODO(), client, "1234asdf", servers.UpdateOpts{Name: &name}).Extract()

Conversation

kayrus commented Sep 27, 2024

Uh oh!

coveralls commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EmilienM commented Oct 4, 2024

Uh oh!

pierreprinetti left a comment

Choose a reason for hiding this comment

Uh oh!

pierreprinetti Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierreprinetti Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierreprinetti Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

pierreprinetti left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Sep 27, 2024 •

edited

Loading

pierreprinetti Oct 4, 2024 •

edited

Loading

pierreprinetti Oct 4, 2024 •

edited

Loading

pierreprinetti Oct 4, 2024 •

edited

Loading