-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add networkID param to PVMInstance's DeleteNetwork() #400
Add networkID param to PVMInstance's DeleteNetwork() #400
Conversation
So normally we can't change the existing definitions, but in this case it seems that in our code this is unused and from looking at the API, it's currently broken. @yussufsh thoughts? What is prompting this change @dharaneeshvrd ? |
@ismirlia I have tried using Mac address of the network that I wanted to remove, but getting below 405 error.
Tried with this change and it went through, so submitted this fix. |
@ismirlia did you get a chance to look at my previous comment? |
@Prajyot-Parab @mkumatag Any input here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@dharaneeshvrd are we trying this for first time? |
Yes I tried this for the first time and got this error, not sure it ever worked. |
that's fair.. in that case @ismirlia can we merge this PR? |
be31944
to
4adc053
Compare
I think it's fine to merge. @yussufsh any thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Network id seems required, as this is a fix for broken api we can merge this.
@ismirlia can you check why the body param with mac is needed? Is it an additional check for deleting the network?
No description provided.