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

Add FWaaS_V2 #1577

Merged
merged 12 commits into from Jun 20, 2023
Merged

Add FWaaS_V2 #1577

merged 12 commits into from Jun 20, 2023

Conversation

Koodt
Copy link
Contributor

@Koodt Koodt commented Jun 5, 2023

Hi there!

Add FWaaS_V2

Close 1/3 part of #115

Add functional-fwaas_v2 workflow.
Add fw_rule_v2 data source, resource, import, acc tests.
Add fw_rule_v2 resource and data sources to DataSourcesMap.
Add fw_rule_v2 website docs.
Add fw_rule_v2 to openstack.erb.

Bump Gophercloud version to 1.4.0.
Update go.sum.

Koodt added 3 commits June 6, 2023 00:31
Add functional-fwaas_v2 worflow.
Exclude FWaaS_V2 tests from networking ACCEPTANCE_TESTS_FILTER.
Add fw_group_v2 data source, resource, import, acc tests.
Add fw_policy_v2 data source, resource, import, acc tests.
Add fw_rule_v2 data source, resource, import, acc tests.
Add FWaaS_V2 resource and data sources to DataSourcesMap.
Add FWaaS_V2 website docs.
Add FWaaS_V2 to openstack.erb.
Bump Gophercloud version to 1.4.0.
Update go.sum.
@Koodt Koodt marked this pull request as ready for review June 6, 2023 12:41
@nikParasyr
Copy link
Member

@Koodt Thanks for this.
CI is not working properly

This is a huge PR (which should have been broken down in multiple PRs). I will have time to review it in a couple of weeks

@nikParasyr
Copy link
Member

@Koodt
Thanks for all the effort. I see that the ci is passing.

This will be hard to review and also not a big fan of adding such a big change in one commit. would it be possible for you to break this into multiple PRs? One PR per resource. So each PR:

  • code for the resource
  • Tests
  • Doc

1st PR should include the CI definition.

I will be able to review them after the 19th

@Koodt
Copy link
Contributor Author

Koodt commented Jun 14, 2023

Groups and policies was removed
Keep only rule's doc, resources, datasource's, tests, CI workflow and golang bump

Copy link
Member

@nikParasyr nikParasyr left a comment

Choose a reason for hiding this comment

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

@Koodt Great PR!
I made some comments about things to fix. Most of them are "nit"/easy to do. Let me know if you need any clarifications somewhere

.github/workflows/functional-fwaas_v2.yml Show resolved Hide resolved
.github/workflows/functional-fwaas_v2.yml Show resolved Hide resolved
openstack/data_source_openstack_fw_rule_v2.go Show resolved Hide resolved
openstack/data_source_openstack_fw_rule_v2.go Show resolved Hide resolved
openstack/data_source_openstack_fw_rule_v2.go Show resolved Hide resolved
openstack/resource_openstack_fw_rule_v2.go Show resolved Hide resolved
openstack/resource_openstack_fw_rule_v2.go Show resolved Hide resolved
openstack/resource_openstack_fw_rule_v2.go Show resolved Hide resolved
openstack/resource_openstack_fw_rule_v2.go Outdated Show resolved Hide resolved
openstack/resource_openstack_fw_rule_v2.go Outdated Show resolved Hide resolved
Copy link
Member

@nikParasyr nikParasyr left a comment

Choose a reason for hiding this comment

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

@Koodt thanks you for this PR and all the work. Also for the patience for the reviews.

This looks good to me.

You can create the rest of the PRs for the fw_v2 resources (incorporate the feedback from this PR please -- such as ValidateFunc etc). I think there should be 2 resources and 2 data_sources left. so you can create 1 PR for each one of them ( 4 in total ), and ill try to review them as soon as possible and I think ill make a release after they merged ( together with a couple PRs that are already open)

@nikParasyr nikParasyr merged commit 4aa7691 into terraform-provider-openstack:main Jun 20, 2023
5 checks passed
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

2 participants