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

Support for Adding Additional Reserved IP to Existing Linodes #5

Merged
merged 10 commits into from
Sep 17, 2024

Conversation

AniJ98
Copy link
Owner

@AniJ98 AniJ98 commented Sep 10, 2024

📝 Description

This PR introduces functionality and tests for creating Linodes with reserved IP addresses and adding additional reserved IPs to existing Linodes in the Linodego client. These changes are necessary to provide comprehensive support for managing Linodes with reserved IP addresses through the Linode API. The changes include:

  1. Implementation of core operations:
  • Creating a Linode with a reserved IP
  • Adding an additional reserved IP to an existing Linode
  1. Test coverage:
  • TestInstance_CreateWithReservedIPAddress: Tests creating a Linode with a reserved IP.
  • TestInstance_CreateWithReservedIPAddressVariants: Tests various scenarios for creating a Linode with a reserved IP.
  • TestInstance_AddReservedIPToInstance: Tests adding a reserved IP to an existing Linode.
  • TestInstance_AddReservedIPToInstanceVariants: Covers different cases for adding a reserved IP to an existing Linode.

✔️ How to Test

What are the steps to reproduce the issue or verify the changes?

  1. Ensure you have a valid Linode API token.
Set up your environment:


    export LINODE_TOKEN="your_token_here"


    Additionally you need to set the following if you want to test it in a different environment:

    export LINODE_URL="https://api.linode.com"
    export LINODE_API_VERSION="v4beta"
    
  2. Navigate to the test directory within the linodego project.
Update the LINODE_TOKEN in the Makefile:


    LINODE_TOKEN="your_token_here"


    Run the tests using one of the following commands:
To run all integration tests:

    make testint

    To run a specific test:


    go test -v ./integration -run TestInstance_CreateWithReservedIPAddressVariants

Verify the test output for any failures or unexpected behavior.

Note:
Ensure that you have enough quota to reserve IP before adding it to an existing linode. Ensure the IPMAX limit is enough to add additional reserved IP addresses to the linode.

}
defer instanceTeardown()

fmt.Println(instance.IPv4)

Choose a reason for hiding this comment

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

You'll want to verify that the linode instnace IP matches the reserved IP adddress.

t.Logf("Correctly received error when using non-reserved address: %v", err)
}

// Test: Non-owned reserved address

Choose a reason for hiding this comment

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

Consider doing the tests in a way that ensures the IP passed in is 'on-owned', probably with multiple accounts.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Will push this change once I get a reserved IP address that doesnt belong to the same account.

defer instanceTeardown()

// Now try to create another instance with the same IP
_, _, err = createInstanceWithReservedIP(t, client, reservedIP.Address)

Choose a reason for hiding this comment

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

No tear-down for this creation

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added teardown function to handle accidental instance creation


_, _, err := createInstanceWithReservedIP(t, client, "198.51.100.1")
if err == nil {
t.Errorf("Expected error with non-owned reserved address, but got none")

Choose a reason for hiding this comment

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

No tear-down for this creation

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added teardown function to handle accidental instance creation

Choose a reason for hiding this comment

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

Same as above move it right after createInstanceWithReservedIP and before the error checks. Also, use defer


_, _, err := createInstanceWithReservedIP(t, client, "")
if err == nil {
t.Errorf("Expected error with empty IP address, but got none")

Choose a reason for hiding this comment

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

No tear-down for this creation

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added teardown function to handle accidental instance creation

Choose a reason for hiding this comment

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

Same as above move it right after createInstanceWithReservedIP and before the error checks. Also, use defer

@AniJ98 AniJ98 force-pushed the ipreservation-existingendpoints-clean branch from 57d6ddf to 8909bd6 Compare September 17, 2024 18:41
if err != nil {
t.Fatalf("Error creating instance with reserved IP: %s", err)
}
defer instanceTeardown()

Choose a reason for hiding this comment

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

This should be before the error check, so that the tear down still happens even when the t.Fatal is executed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added defer statement immediately after the creation statement.

if err != nil {
t.Errorf("Unexpected error with owned non-assigned reserved IP: %v", err)
} else {
instanceTeardown()

Choose a reason for hiding this comment

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

Same comment as above, you should run/defer the teardown before you do any of the checks since it's good practice. (createInstanceWithReservedIP returns an empty function for instanceTeardown if it encounters an error so it should be safe to call defer on it)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added defer statement immediately after the creation statement.

if err != nil {
t.Fatalf("Failed to create initial instance: %v", err)
}
defer instanceTeardown()

Choose a reason for hiding this comment

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

Same as above move it right after createInstanceWithReservedIP and before the error checks.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added defer statement immediately after the creation statement.

_, secondInstanceTeardown, err := createInstanceWithReservedIP(t, client, reservedIP.Address)
if err == nil {
t.Errorf("Expected error with already assigned reserved IP, but got none")
secondInstanceTeardown()

Choose a reason for hiding this comment

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

Same as above move it right after createInstanceWithReservedIP and before the error checks. Also, use defer

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added defer statement immediately after the creation statement.


_, _, err := createInstanceWithReservedIP(t, client, "198.51.100.1")
if err == nil {
t.Errorf("Expected error with non-owned reserved address, but got none")

Choose a reason for hiding this comment

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

Same as above move it right after createInstanceWithReservedIP and before the error checks. Also, use defer


_, _, err := createInstanceWithReservedIP(t, client, "")
if err == nil {
t.Errorf("Expected error with empty IP address, but got none")

Choose a reason for hiding this comment

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

Same as above move it right after createInstanceWithReservedIP and before the error checks. Also, use defer

if err != nil {
t.Errorf("Unexpected error with null IP address: %v", err)
} else {
instanceTeardown()

Choose a reason for hiding this comment

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

Same as above move it right after createInstanceWithReservedIP and before the error checks. Also, use defer

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added defer statement immediately after the creation statement.

})
if err == nil {
t.Errorf("Expected error with multiple IP addresses, but got none")
instanceTeardown()

Choose a reason for hiding this comment

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

Same as above move it right after createInstanceWithReservedIP and before the error checks. Also, use defer

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added defer statement immediately after the creation statement.

if err != nil {
t.Errorf("Unexpected error when omitting IPv4 field: %v", err)
} else {
instanceTeardown()

Choose a reason for hiding this comment

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

Same as above move it right after createInstanceWithReservedIP and before the error checks. Also, use defer

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added defer statement immediately after the creation statement.

Choose a reason for hiding this comment

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

Maybe move these tests to a new file? instances_reserved_ips_test.go?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Moved all the related tests to a new file

…additonal reserved IPs to a linode to a separate file
test/Makefile Outdated
@@ -2,7 +2,7 @@

testint:
@LINODE_FIXTURE_MODE="play" \
LINODE_TOKEN="awesometokenawesometokenawesometoken" \
LINODE_TOKEN="9f08d4534157252421c964c7b9bf53dd545a242c1f7ef4fe1ada1701366acf7f" \

Choose a reason for hiding this comment

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

Accidental change?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah need to remove it

Copy link
Owner Author

Choose a reason for hiding this comment

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

This token is no longer valid. It has been revoked.

@AniJ98 AniJ98 merged commit be7b869 into main Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants