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.

Sign up for GitHub

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

Jump to bottom

Add implementation for vapp network static routing rules resource #520

Merged
merged 34 commits into from
Jun 26, 2020
Merged

Add implementation for vapp network static routing rules resource #520

merged 34 commits into from
Jun 26, 2020

Conversation

vbauzys
Copy link
Contributor

@vbauzys vbauzys commented Jun 18, 2020

Dep: vmware/go-vcloud-director#318
Ref: #329

New resource to manage vapp network static routing rules

This PR is derived from #511 and #517 so shows that PR code too

@vbauzys vbauzys requested review from dataclouder and lvirbalas and removed request for dataclouder June 18, 2020 10:40
@vbauzys vbauzys self-assigned this Jun 18, 2020
@lvirbalas lvirbalas requested a review from Didainius June 23, 2020 10:23
lvirbalas
lvirbalas requested changes Jun 23, 2020
View reviewed changes
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

A pass at docs.

website/docs/r/vapp_static_routing.html.markdown Outdated Show resolved Hide resolved
website/docs/r/vapp_static_routing.html.markdown Outdated Show resolved Hide resolved
website/docs/r/vapp_static_routing.html.markdown Outdated Show resolved Hide resolved
website/docs/r/vapp_static_routing.html.markdown

## Argument Reference

The following arguments are supported:
Copy link
Collaborator

Choose a reason for hiding this comment

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

If API returns it, I think we need to add a computed read-only attribute for the "Router external IP" as in the GUI:

Screenshot 2020-06-23 at 13 36 41

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had same idea, but this information from different API calls.

website/docs/r/vapp_static_routing.html.markdown Outdated Show resolved Hide resolved
website/docs/r/vapp_static_routing.html.markdown Outdated Show resolved Hide resolved
website/docs/r/vapp_static_routing.html.markdown Show resolved Hide resolved
@ghost ghost added size/M and removed size/XXL labels Jun 23, 2020
@ghost ghost added size/XL and removed size/M labels Jun 23, 2020
vbauzys added 5 commits June 23, 2020 18:10
@vbauzys
Merge branch 'master' into vapp-routing
344ed7d 
# Conflicts:
#	CHANGELOG.md
#	go.mod
#	go.sum
#	vendor/github.com/vmware/go-vcloud-director/v2/govcd/vapp_network.go
#	vendor/modules.txt
@vbauzys
Fixes after merge
3cb870b
@vbauzys
Fixes
712e752
@vbauzys
Improve docs
e49e8fa
@vbauzys
Improve docs
c6ce55d
dataclouder
dataclouder suggested changes Jun 23, 2020
View reviewed changes
Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

First pass. More after testing

vcd/resource_vcd_vapp_static_routing_test.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing_test.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing_test.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing_test.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing_test.go Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing_test.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing.go Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing.go Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing.go Show resolved Hide resolved
Didainius
Didainius reviewed Jun 24, 2020
View reviewed changes
Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

Thanks!
A few comments inline with explicit interest in consistent enabled/disabled handling for all vApp resources.

Also tests are broken at the moment because of field name:
=== RUN TestAccVcdVappStaticRouting
TestAccVcdVappStaticRouting: testing.go:654: Step 0 error: config is invalid: Unsupported argument: An argument named "enable" is not e
xpected here. Did you mean "enabled"?
--- FAIL: TestAccVcdVappStaticRouting (0.03s)

website/docs/r/vapp_static_routing.html.markdown Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing.go Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_static_routing.go Show resolved Hide resolved
@vbauzys
Copy link
Contributor Author

vbauzys commented Jun 25, 2020

Thanks!
A few comments inline with explicit interest in consistent enabled/disabled handling for all vApp resources.

Also tests are broken at the moment because of field name:
=== RUN TestAccVcdVappStaticRouting
TestAccVcdVappStaticRouting: testing.go:654: Step 0 error: config is invalid: Unsupported argument: An argument named "enable" is not e
xpected here. Did you mean "enabled"?
--- FAIL: TestAccVcdVappStaticRouting (0.03s)

Test runs now fine.

@ghost ghost added size/XXL and removed size/XL labels Jun 25, 2020
lvirbalas
lvirbalas reviewed Jun 26, 2020
View reviewed changes
website/docs/r/vapp_firewall_rules.html.markdown Show resolved Hide resolved
vcd/resource_vcd_vapp_firewall_rules.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_nat_rules.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_nat_rules.go Outdated Show resolved Hide resolved
vcd/resource_vcd_vapp_nat_rules_test.go Show resolved Hide resolved
lvirbalas
lvirbalas approved these changes Jun 26, 2020
View reviewed changes
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

32 commits and 97 conversation messages to perfection. LGTM now! :D Thank you!!!

dataclouder
dataclouder approved these changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

LGTM

@vbauzys vbauzys merged commit 3b68fdd into vmware:master Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datacharmer datacharmer datacharmer left review comments

@Didainius Didainius Didainius approved these changes

@dataclouder dataclouder dataclouder approved these changes

@lvirbalas lvirbalas lvirbalas approved these changes

Assignees

@vbauzys vbauzys

Labels
dependencies documentation size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vbauzys @datacharmer @lvirbalas @dataclouder @Didainius