Add tags support for Neutron Networks #454
Conversation
hardys
changed the title
|
Marking this WIP because the trunk patch from @celebdor has some build failures breaking the tests - when that's resolved I'll rebase this but it's otherwise ready for review. If the approach taken here is OK I'll go ahead and push PRs/patches for the remaining resources using a similar pattern @jtopjian would you prefer one PR per resource or adding commits to this PR for all resources? I assume the former similar to gophercloud but wanted to check. |
ghost
added
the
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
openstack/util.go
Outdated
| return | ||
| } | ||
|
|
||
| func testAccCheckNetworkingV2Tags(name string, key string, tags []string) resource.TestCheckFunc { |
There was a problem hiding this comment.
Small nit - now this function has been renamed we could probably drop the key parameter, as it'll always be "tags" I think?
|
Build failed.
|
hardys
force-pushed
the
neutron_tags
branch
2 times, most recently
from
7a933dd
to
e2e747e
Compare
|
Build failed.
|
hardys
changed the title
|
Build succeeded.
|
|
@jtopjian when you have time this is ready for review I think - I've rebased on the latest trunk patch and the tests are all now green. I've also started staging the remaining patches at https://github.com/hardys/terraform-provider-openstack/tree/neutron_tags_wip but I'll defer pushing PRs until we've iterated on this patch as I assume any review feedback will be common to all in the series and I don't want to waste CI cycles. Thanks! |
|
Note I've added the test for network to the per-resource test, but that will end up a little inefficient as we end up creating a network for other tests e.g subnet/port etc as well. This is kind of a side-effect of the one PR per resource requirement I think, but I could probably refactor the test at the end of the series to a single template which creates all-the-resources that support tags? |
|
Rebased on master now the trunk patch landed, also fixed a minor issue in the docs markdown. |
|
Build succeeded.
|
|
@hardys I'm going to be away for the next few days and might not get a chance to review this in detail. I'll do it as soon as I can, though.
Good idea :) |
Ok I'll look into it as the current tests are a bit slow: I guess this duplication isn't unique to the tags test, but if I refactor things to have a new e.g TestAccNetworkingV2_tags common test we can at least remove the Network/Subnet test overhead (since those are already created for the Port test) which is over 1min and I suspect using a single terraform invocation/template will reduce the overall test overhead quite a bit as well. |
|
Ok I refactored the tests for the entire series and it's a lot faster: |
|
Ok the latest revision has a separate test for tags, and the series at https://github.com/hardys/terraform-provider-openstack/commits/neutron_tags_wip adds to the test which speeds things up considerably. When this passes CI I'll push a couple more of the pending patches as dependent PRs as I think that neutron_tags_wip branch is all ready for review now. |
|
Build failed.
|
|
recheck |
|
Build succeeded.
|
| @@ -112,6 +114,7 @@ The following attributes are exported: | |||
| * `tenant_id` - See Argument Reference above. | |||
| * `admin_state_up` - See Argument Reference above. | |||
| * `availability_zone_hints` - See Argument Reference above. | |||
| * `tags` - Argument Reference above. | |||
There was a problem hiding this comment.
Let's keep it similar to other attributes: "See Argument Reference above.".
This allows us to set/get tags for networks via terraform templates, and requires the recently added changes to gophercloud that enable the relevant features of the Neutron APIs to be accessed. Note the test is added in a new resource_openstack_networking_tags_v2_test.go file, so that we can reduce duplication later in the series e.g subnet/port/trunk resources can share the same template which should be faster. Issue: terraform-provider-openstack#453
|
@ozerovandrei thanks for the review - I fixed the docs nit in the latest revision. |
|
Build succeeded.
|
This adds support for tags to networks, note this is branched from PR #446 since that already adds initial support for tags for the trunk resource, and a couple of functions are reused here.
Subsequent PRs will add support for the other neutron resources which support standard attribute tags as described in Issue #453
Issue #453