Skip to content
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

fix: [BREAKING] Treat ip_ranges as a pointer in config interface update options #464

Merged

Conversation

lgarber-akamai
Copy link
Contributor

@lgarber-akamai lgarber-akamai commented Mar 4, 2024

📝 Description

This change resolves an issue that prevented users from updating the interface ip_ranges field with an explicitly empty list. This works by treating ip_ranges as a pointer so it will be only omitted if the pointer is nil, as opposed to if the slice is nil OR the slice is empty.

Resolves #462

✔️ How to Test

The following test steps assume you have pulled down this PR locally.

Integration Testing

make ARGS="-run TestInstance_ConfigInterface_Update" fixtures

Manual Testing

  1. In a linodego sandbox environment (e.g. dx-devenv), run the following:
package main

import (
	"context"
	"fmt"
	"github.com/linode/linodego"
	"log"
	"os"
)

func main() {
	ctx := context.Background()

	client := linodego.NewClient(nil)
	client.SetToken(os.Getenv("LINODE_TOKEN"))

	vpc, err := client.CreateVPC(ctx, linodego.VPCCreateOptions{
		Label:  "test-vpc",
		Region: "us-mia",
		Subnets: []linodego.VPCSubnetCreateOptions{
			{
				Label: "test-subnet",
				IPv4:  "10.0.0.0/24",
			},
		},
	})
	if err != nil {
		log.Fatal(err)
	}

	defer func() {
		client.DeleteVPC(ctx, vpc.ID)
	}()

	inst, err := client.CreateInstance(
		ctx,
		linodego.InstanceCreateOptions{
			Region: "us-mia",
			Label:  "test-inst",
			Type:   "g6-nanode-1",
		},
	)
	if err != nil {
		log.Fatal(err)
	}
	defer func() {
		client.DeleteInstance(ctx, inst.ID)
	}()

	cfg, err := client.CreateInstanceConfig(
		ctx,
		inst.ID,
		linodego.InstanceConfigCreateOptions{
			Label:   "test-config",
			Devices: linodego.InstanceConfigDeviceMap{},
			Interfaces: []linodego.InstanceConfigInterfaceCreateOptions{
				{
					Purpose:  linodego.InterfacePurposeVPC,
					SubnetID: &vpc.Subnets[0].ID,
					IPRanges: []string{"10.0.0.5/32"},
				},
			},
		},
	)
	if err != nil {
		log.Fatal(err)
	}

	fmt.Println("ip_ranges before update:", cfg.Interfaces[0].IPRanges)

	updatedIPRanges := make([]string, 0)

	iface, err := client.UpdateInstanceConfigInterface(
		ctx,
		inst.ID,
		cfg.ID,
		cfg.Interfaces[0].ID,
		linodego.InstanceConfigInterfaceUpdateOptions{
			IPRanges: &updatedIPRanges,
		},
	)
	if err != nil {
		log.Fatal(err)
	}

	fmt.Println("ip_ranges after update: ", iface.IPRanges)
}
  1. Ensure no errors are raised and the output looks similar to the following:
ip_ranges before update: [10.0.0.5/32]
ip_ranges after update:  []

@lgarber-akamai lgarber-akamai changed the title Implement ip_ranges workaround fix: Implement ip_ranges workaround Mar 4, 2024
@lgarber-akamai lgarber-akamai marked this pull request as ready for review March 4, 2024 21:24
@lgarber-akamai lgarber-akamai requested a review from a team as a code owner March 4, 2024 21:24
@lgarber-akamai lgarber-akamai requested review from jriddle-linode and zliang-akamai and removed request for a team March 4, 2024 21:24
@lgarber-akamai lgarber-akamai changed the title fix: Implement ip_ranges workaround fix: Allow specifying an explicitly empty ip_ranges when updating a config interface Mar 4, 2024
@lgarber-akamai lgarber-akamai changed the title fix: Allow specifying an explicitly empty ip_ranges when updating a config interface fix: Allow specifying explicitly empty ip_ranges when updating a config interface Mar 4, 2024
Copy link
Member

@zliang-akamai zliang-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be making IPRanges as a pointer, but that would be a breaking change for the users? I think it might be fine since we are still in the early phase of VPC? Or maybe we can schedule it to linodego v2?

@lgarber-akamai
Copy link
Contributor Author

lgarber-akamai commented Mar 5, 2024

Another option would be making IPRanges as a pointer, but that would be a breaking change for the users? I think it might be fine since we are still in the early phase of VPC? Or maybe we can schedule it to linodego v2?

@zliang-akamai That's a good point! I wrote this with avoiding a breaking change in mind but it might make sense to transition this to a pointer now or in v2 for consistency with the rest of the codebase.

@lgarber-akamai lgarber-akamai changed the title fix: Allow specifying explicitly empty ip_ranges when updating a config interface fix: [BREAKING] Treat ip_ranges as a pointer in config interface update options Mar 5, 2024
Copy link
Contributor

@yec-akamai yec-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tests passed locally

@jriddle-linode jriddle-linode added the breaking-change for breaking changes in the changelog. label Mar 5, 2024
Copy link
Contributor

@jriddle-linode jriddle-linode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lgarber-akamai lgarber-akamai added the bugfix for any bug fixes in the changelog. label Mar 6, 2024
@lgarber-akamai lgarber-akamai merged commit 09468a5 into linode:main Mar 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change for breaking changes in the changelog. bugfix for any bug fixes in the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: unable to empty ip_ranges for VPC interface once set
4 participants