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

refactor(eip): Refactor eip using egoscale v3 #642

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mlec1
Copy link
Contributor

@mlec1 mlec1 commented Oct 6, 2024

Update the elastic ip using Egoscale v3.
Some stuff are not working yet: I put some comments in the code, you will see.

Checklist

(For exoscale contributors)

  • Changelog updated (under Unreleased block)
  • Testing

Testing

Create, delete, update ip,...

Comment on lines +110 to +115
// Comment for reviewer:
// Is there a way to get the created Elastic IP address or UUID ???
// Listing all of them and comparing Addressfamily, Description,... doesn't really garantee uniquess
// TODO: Remove comment before merging
ElasticIP: "",
Zone: c.Zone,
Copy link
Member

Choose a reason for hiding this comment

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

yes you need to get the op after waiting and there is the op.Reference.ID check if reference not nil before using

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to get the op from the decorateAsyncOperations ??
As this function is only returning an error if it happens ??!!

Comment on lines +80 to +86
// Message for reviewer:
// It always returns the list of all instances. Why ???
// When no instance attached to Elastic IP, should returned an empty list
// When some instances attached to elastic IP, should returns only the attached instances
// Am I doing something wrong here ?? Is the ip-address parameter taking the Elastic IP into account ??
// TODO: remove comment before merging
instances, err := client.ListInstances(ctx, v3.ListInstancesWithIPAddress(elasticIPID))
Copy link
Member

Choose a reason for hiding this comment

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

I'll try to reproduce, thanks for reporting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The list-instance option is still not working.
My guess is that in egoscale, the get parameter being added is ipAddress https://github.com/exoscale/egoscale/blob/v3.1.7/v3/operations.go#L9770 when on the openapi spec the query parameter should be ip-address.

Same for manager-id and manager-type

It is just a supposition.

Copy link
Member

@pierre-emmanuelJ pierre-emmanuelJ left a comment

Choose a reason for hiding this comment

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

It's a good change!!, we will do another review on it

cmd/elastic_ip_delete.go Outdated Show resolved Hide resolved
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.

2 participants