Skip to content

lbaas: l7policies: don't omit invert: bool=false#1314

Merged
jtopjian merged 1 commit intogophercloud:masterfrom
kayrus:kayrus/no-revert-omitempty
Dec 5, 2018
Merged

lbaas: l7policies: don't omit invert: bool=false#1314
jtopjian merged 1 commit intogophercloud:masterfrom
kayrus:kayrus/no-revert-omitempty

Conversation

@kayrus
Copy link
Copy Markdown
Contributor

@kayrus kayrus commented Oct 30, 2018

When the rule was initially created with invert=true, it is not possible to update it to invert=false, because json engine omits false value.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 31, 2018

Build failed.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Oct 31, 2018

@kayrus Thank you for submitting this, but the comment I made previously still applies here: #1313 (comment)

@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Oct 31, 2018

@jtopjian, I'll take care about your suggestions soon.

I've noticed that there is a lot of inconsistencies like this. E.g. for the admin_state_up values.

As far as I understood there could be only two cases:

  • using the pointer AdminStateUp *bool json:"admin_state_up,omitempty"``
  • using the pure bool AdminStateUp bool json:"admin_state_up"``
$ grep -r ' bool `json:' | grep omit
compute/v2/extensions/quotasets/requests.go:    Force bool `json:"force,omitempty"`
networking/v2/extensions/trunks/results.go:     AdminStateUp bool `json:"admin_state_up,omitempty"`
networking/v2/extensions/subnetpools/requests.go:       Shared bool `json:"shared,omitempty"`
networking/v2/extensions/subnetpools/requests.go:       IsDefault bool `json:"is_default,omitempty"`
sharedfilesystems/v2/shares/results.go: IsPublic bool `json:"is_public,omitempty"`
blockstorage/v3/volumes/requests.go:    Multiattach bool `json:"multiattach,omitempty"`
blockstorage/extensions/quotasets/requests.go:  Force bool `json:"force,omitempty"`
blockstorage/extensions/volumeactions/requests.go:      Force bool `json:"force,omitempty"`
loadbalancer/v2/l7policies/requests.go: Invert bool `json:"invert,omitempty"`
loadbalancer/v2/l7policies/requests.go: Invert bool `json:"invert,omitempty"`
$ grep -r ' \*bool `json:' | grep -v omit
networking/v2/extensions/vpnaas/services/requests.go:   AdminStateUp *bool `json:"admin_state_up"`

Does it make sense to fix that as well?

Here are more details golang/go#13284

@kayrus kayrus changed the title Don't omit invert=false Don't omit bool=false Oct 31, 2018
@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Oct 31, 2018

same behavior with empty strings (e.g Name or Description fields)

@jtopjian
Copy link
Copy Markdown
Contributor

@kayrus It's really best to open an issue to discuss these changes rather than first submit a Pull Request for one change and then discuss further changes in the same Pull Request.

As for the validity of these changes, it's possible there are some parts of the code that need updated to use pointers and it's possible that the current code is correct and the various OpenStack API services are not consistent. We will need to see references to actual Python code in order to determine this.

All of this (and more!) is covered in our contributor tutorial 😄

@kiwik
Copy link
Copy Markdown
Contributor

kiwik commented Nov 5, 2018

recheck

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Nov 5, 2018

Build failed.

@jtopjian
Copy link
Copy Markdown
Contributor

@kayrus If you remove omitempty from these fields, then they will always send false, even if the field wasn't specified. This is why Travis is reporting an error.

Instead of setting omitempty, you will need to change the field from bool to *bool. If you grep the Gophercloud repo for *bool, you'll see a lot of examples to go off of.

@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Nov 30, 2018

@jtopjian I'm aware about the pointer. I didn't want to break the compatibility. That is why I created a #1332 issue to discuss changes.

@jtopjian
Copy link
Copy Markdown
Contributor

Sure - I'll jump over there :)

@kayrus kayrus closed this Dec 3, 2018
@kayrus kayrus reopened this Dec 3, 2018
@kayrus kayrus force-pushed the kayrus/no-revert-omitempty branch from 46b0631 to 5b26234 Compare December 3, 2018 09:22
@coveralls
Copy link
Copy Markdown

coveralls commented Dec 3, 2018

Coverage Status

Coverage remained the same at 79.621% when pulling 15d6cd8 on kayrus:kayrus/no-revert-omitempty into fac2636 on gophercloud:master.

@kayrus kayrus force-pushed the kayrus/no-revert-omitempty branch from 5b26234 to ae60630 Compare December 3, 2018 09:31
@kayrus kayrus force-pushed the kayrus/no-revert-omitempty branch from ae60630 to 15d6cd8 Compare December 3, 2018 09:50
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 3, 2018

Build failed.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Dec 4, 2018

recheck

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 4, 2018

Build succeeded.

@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Dec 4, 2018

Good news. I did review all the grepped results and surprisingly it appeared that the only one bool related issue was in the l7policies/requests.go.

@kayrus kayrus changed the title Don't omit bool=false lbaas: l7policies: don't omit invert: bool=false Dec 4, 2018
@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Dec 5, 2018

@kayrus Nice - thank you for reviewing everything :)

Is this ready to review/merge?

@kayrus
Copy link
Copy Markdown
Contributor Author

kayrus commented Dec 5, 2018

@jtopjian yes.

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

@jtopjian jtopjian merged commit e1cdd49 into gophercloud:master Dec 5, 2018
@kayrus kayrus deleted the kayrus/no-revert-omitempty branch December 5, 2018 08:04
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.

4 participants