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

Adds VirtualNetwork settings #2

Merged
merged 1 commit into from
Sep 12, 2017
Merged

Adds VirtualNetwork settings #2

merged 1 commit into from
Sep 12, 2017

Conversation

rossf7
Copy link
Contributor

@rossf7 rossf7 commented Sep 7, 2017

Towards giantswarm/azure-operator/issues/6

This PR adds the VirtualNetwork settings so they can be used in the Network Setup ARM template.

@rossf7 rossf7 self-assigned this Sep 7, 2017
package azure

type VirtualNetwork struct {
CIDR string `json:"cidr" yaml:"cidr"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be possible to update the json and yaml tags to match the ones used in the ARM templates?

I can also update my own PR to match your tags without any issues :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to use this format because Kubernetes uses it for serialising and deserialising the TPOs.

I like having the ARM templates matching for consistency. So if you're OK to change that would be good. But if there are naming conventions for the ARM templates maybe we should follow those?

Copy link

@fgimenez fgimenez left a comment

Choose a reason for hiding this comment

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

LGTM

@rossf7 rossf7 merged commit cda860a into master Sep 12, 2017
@rossf7 rossf7 deleted the vnet branch September 12, 2017 13:34
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.

None yet

3 participants