[FWaaS_V2]: Add FW Group V2 resource #1588
Conversation
Add resourceFWGroupV2 CRUD Add resourceFWGroupV2 tests Add resourceFWGroupV2 docs Add import_openstack_fw_group_v2_test Enable openstack_fw_group_v2 in provider.go
|
|
||
| _, err := stateConf.WaitForStateContext(ctx) | ||
| if err != nil { | ||
| return diag.Errorf("Error waiting for openstack_fw_group_v2 %s to become active: %s", d.Id(), err) |
There was a problem hiding this comment.
| return diag.Errorf("Error waiting for openstack_fw_group_v2 %s to become active: %s", d.Id(), err) | |
| return diag.Errorf("Error waiting for openstack_fw_group_v2 %s to become deleted: %s", d.Id(), err) |
There was a problem hiding this comment.
We don't delete the group, but wait for its status after deleting a policy from it.
openstack/fw_group_v2.go
Outdated
| return func() (interface{}, string, error) { | ||
| var group groups.Group | ||
|
|
||
| err := groups.Get(networkingClient, id).ExtractIntoStructPtr(&group, "firewall_group") |
There was a problem hiding this comment.
Is there a reason to call the ExtractIntoStructPtr? In the fwGroupV2DeleteFunc you're just calling Extract.
Maybe it makes sense to combine fwGroupV2RefreshFunc and fwGroupV2DeleteFunc into the one fwGroupV2RefreshFunc?
There was a problem hiding this comment.
Call the ExtractIntoStructPtr is needed to return the status dynamically.
I think returning nill error after gophercloud.ErrDefault404 when we create or update firewall group would be a bad idea.
So, I decided to separate them
There was a problem hiding this comment.
return the status dynamically
not clear what exactly does it mean :\
There was a problem hiding this comment.
Understood your question.
It's V1 legacy. Align style fwGroupV2RefreshFunc with fwGroupV2DeleteFunc
|
|
||
| groupcreateOpts := groups.CreateOpts{ | ||
| Name: d.Get("name").(string), | ||
| TenantID: d.Get("tenant_id").(string), |
There was a problem hiding this comment.
Does it make sense to add ProjectID here as well?
|
|
||
| _, err = stateConf.WaitForStateContext(ctx) | ||
| if err != nil { | ||
| return diag.Errorf("Error waiting for openstack_fw_firewall_v2 %s to Delete: %s", d.Id(), err) |
There was a problem hiding this comment.
| return diag.Errorf("Error waiting for openstack_fw_firewall_v2 %s to Delete: %s", d.Id(), err) | |
| return diag.Errorf("Error waiting for openstack_fw_firewall_v2 %s to delete: %s", d.Id(), err) |
| "status": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Computed: true, | ||
| }, |
There was a problem hiding this comment.
This is a computed attribute. Also please move it to the bottom.
| "status": { | |
| Type: schema.TypeString, | |
| Optional: true, | |
| Computed: true, | |
| }, | |
| "status": { | |
| Type: schema.TypeString, | |
| Computed: true, | |
| }, |
| * `status` - (Optional) Administrative up/down status for the firewall | ||
| group (must be "true" or "false" if provided - defaults to "true"). | ||
| Changing this updates the `admin_state_up` of an existing firewall group. |
There was a problem hiding this comment.
this should be removed, since it's a computed attribute. Also please add a corresponding description in the attribute reference.
There was a problem hiding this comment.
The status is still not removed.
There was a problem hiding this comment.
Shared was removed, not status. Lol.
Fix
| wants to create a firewall group for another tenant. Changing this creates | ||
| a new firewall group. | ||
|
|
||
| * `project_id` - (Optional) The owner of the firewall group. Required if admin |
There was a problem hiding this comment.
Please give a description about the difference of project_id and tenant_id. Do you think it makes sense to add ConflictsWith in the schema definition?
There was a problem hiding this comment.
I also think ConflictsWith might be good.
There was a problem hiding this comment.
Description - ok
Why tenant conflict with project? It's the same.
They will only conflict if there are different values.
In this case, we need the ConflictsWith
There was a problem hiding this comment.
I think in its current form it can be a bit confusing for the user:
- they might either define both with the same value, duplication for no reason
- they might define them with a different value which will lead to an error
So ConflictsWith solves the above and then it can be documented as well to make it clearer for the user that only one is required (and accepted).
There was a problem hiding this comment.
I completely agree
Done
| wants to create a firewall group for another tenant. Changing this creates | ||
| a new firewall group. | ||
|
|
||
| * `project_id` - (Optional) The owner of the firewall group. Required if admin |
There was a problem hiding this comment.
I also think ConflictsWith might be good.
| if hasChange { | ||
| log.Printf("[DEBUG] openstack_fw_group_v2 %s update options: %#v", d.Id(), updateOpts) | ||
|
|
||
| err = groups.Update(networkingClient, d.Id(), updateOpts).Err |
There was a problem hiding this comment.
Please use Extract() here.
There was a problem hiding this comment.
Done
Also for rules and policies
|
@kayrus @nikParasyr all checks have passed, all conversations was answered. |
|
@Koodt are you planning to add a data source for FW groups? |
|
@kayrus yes |
Add resourceFWGroupV2 CRUD
Add resourceFWGroupV2 tests
Add resourceFWGroupV2 docs
Add import_openstack_fw_group_v2_test
Enable openstack_fw_group_v2 in provider.go
Close part of #115