Skip to content

Adding functions to modify network settings and policies. Adding RemoteSubnetRoutePolicy.#358

Merged
jterry75 merged 6 commits intomicrosoft:masterfrom
pradipd:remotesubnet
Oct 19, 2018
Merged

Adding functions to modify network settings and policies. Adding RemoteSubnetRoutePolicy.#358
jterry75 merged 6 commits intomicrosoft:masterfrom
pradipd:remotesubnet

Conversation

@pradipd
Copy link
Copy Markdown
Contributor

@pradipd pradipd commented Oct 19, 2018

No description provided.

Comment thread hcn/hcnnetwork_test.go
network.AddPolicy(*remoteSubnetRoutePolicy)

//Reload the network object from HNS.
network, err = GetNetworkByID(network.Id)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not fond of this pattern. We shouldn't have to reload the network after modifying. After updating the network (or endpoint) the network should be automatically updated. However, this behavior is consistent with the Endpoint API's.
I'd prefer we change this behavior in a separate PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like your proposal.

Comment thread hcn/hcnnetwork.go
Comment thread hcn/hcnnetwork.go
Comment thread hcn/hcnpolicy.go Outdated
Copy link
Copy Markdown
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

Just a few comments.

@jterry75
Copy link
Copy Markdown
Contributor

@madhanrm - Want to give a quick review?

@pradipd
Copy link
Copy Markdown
Contributor Author

pradipd commented Oct 19, 2018

@jterry75 I've emailed the team about the PR. At some point in time I'll learn there GitHub aliases and add them to the PR.

Comment thread hcn/hcnnetwork_test.go Outdated
Comment thread hcn/hcnnetwork_test.go Outdated
Comment thread hcn/hcnutils_test.go Outdated
return loadBalancer.Create()
}

func HcnCreateRemoteSubnetRoute() (*PolicyNetworkRequest, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Can you rename this to HcnCreateTestRemoteSubnetRoute

@jterry75
Copy link
Copy Markdown
Contributor

@madhanrm - Let me know when this LGT you and I'll merge. This is really your code

@pradipd
Copy link
Copy Markdown
Contributor Author

pradipd commented Oct 19, 2018

Forgot to rerun the tests after all the changes.

@madhanrm
Copy link
Copy Markdown
Contributor

LGTM

@pradipd
Copy link
Copy Markdown
Contributor Author

pradipd commented Oct 19, 2018

Validated all the tests run.
Out of curiosity, how come we don't run the tests as a CI gate?

@pradipd
Copy link
Copy Markdown
Contributor Author

pradipd commented Oct 19, 2018

@erfrimod: FYI

@jterry75 jterry75 merged commit 04fd311 into microsoft:master Oct 19, 2018
@madhanrm
Copy link
Copy Markdown
Contributor

This test would run only on RS5+

dcantah pushed a commit to dcantah/hcsshim that referenced this pull request Mar 17, 2021
Remove v2 dependency on v1 for host processes
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.

4 participants