Skip to content

[wip] FWaaS v2.0 Rule List#1783

Merged
jtopjian merged 23 commits intogophercloud:masterfrom
Elethiomel:fwaasv2_rule_list
Nov 29, 2019
Merged

[wip] FWaaS v2.0 Rule List#1783
jtopjian merged 23 commits intogophercloud:masterfrom
Elethiomel:fwaasv2_rule_list

Conversation

@coveralls
Copy link
Copy Markdown

coveralls commented Nov 27, 2019

Coverage Status

Coverage increased (+0.01%) to 76.99% when pulling 4e8d6bc on Elethiomel:fwaasv2_rule_list into 978be38 on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Nov 27, 2019

Build failed.

@Elethiomel
Copy link
Copy Markdown
Contributor Author

recheck

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Nov 27, 2019

Build failed.

  • gophercloud-unittest : SUCCESS in 3m 04s
  • gophercloud-acceptance-test : RETRY_LIMIT in 13m 41s
  • gophercloud-acceptance-test-ironic : RETRY_LIMIT in 13m 05s

@Elethiomel
Copy link
Copy Markdown
Contributor Author

recheck

@Elethiomel
Copy link
Copy Markdown
Contributor Author

@jtopjian Openlab checks failing. Doesn't seem to be related to my code though :)

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Nov 27, 2019

Build succeeded.

@Elethiomel
Copy link
Copy Markdown
Contributor Author

@jtopjian There we go. Recheck successful :)

Copy link
Copy Markdown
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@Elethiomel Thanks!

I've left two comments in regards to ListOpts. And for some reason, Update is getting mixed in here. Please let me know if you have any questions.

DestinationIPAddress string `q:"destination_ip_address"`
SourcePort string `q:"source_port"`
DestinationPort string `q:"destination_port"`
Enabled bool `q:"enabled"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By default, Gophercloud does not set zero-values in the query strings. Using bool, we'll only ever be able to query ?enabled=true. We should change this to *bool so we can do ?enabled=false.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should be resolved in commit 4e8d6bc

DestinationPort string `q:"destination_port"`
Enabled bool `q:"enabled"`
ID string `q:"id"`
Shared bool `q:"shared"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should be resolved in commit 4e8d6bc

return
}

// UpdateOptsBuilder is the interface options structs have to satisfy in order
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this PR might need rebased off of Gophercloud's master branch. Update has already been merged: https://github.com/gophercloud/gophercloud/blob/master/openstack/networking/v2/extensions/fwaas_v2/rules/requests.go#L93

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops :D Good catch, thanks!

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Nov 28, 2019

Build succeeded.

@Elethiomel
Copy link
Copy Markdown
Contributor Author

@jtopjian Look OK to you?

@Elethiomel Elethiomel requested a review from jtopjian November 29, 2019 16:44
@jtopjian
Copy link
Copy Markdown
Contributor

@Elethiomel I apologize for the delay. I will review this as soon as I get a chance.

Copy link
Copy Markdown
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

LGTM - thank you!

@jtopjian jtopjian merged commit 4654070 into gophercloud:master Nov 29, 2019
@jtopjian
Copy link
Copy Markdown
Contributor

@Elethiomel It looks like you've got a good understanding of how to implement this, including understanding how it relates to the Python-based service code.

Please feel free to combine CRUD into a single PR for the rest of the resources.

@Elethiomel Elethiomel deleted the fwaasv2_rule_list branch December 2, 2019 17:06
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.

3 participants