Skip to content

Identity v3 Region update#594

Merged
jtopjian merged 2 commits intogophercloud:masterfrom
dklyle:region-update
Nov 7, 2017
Merged

Identity v3 Region update#594
jtopjian merged 2 commits intogophercloud:masterfrom
dklyle:region-update

Conversation

@dklyle
Copy link
Copy Markdown
Contributor

@dklyle dklyle commented Nov 3, 2017

Note that ID cannot be updated even though it is settable on create.

There is also currently an issue with updating the Extra field. This I believe is
a bug in keystone. I have posted a bug and patch to fix. https://bugs.launchpad.net/keystone/+bug/1729933

For #583
Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

API:
https://developer.openstack.org/api-ref/identity/v3/#update-region

Schema:
https://github.com/openstack/keystone/blob/2764b49bc209245276da31693ef31f1525cf17d8/keystone/catalog/backends/sql.py#L175

Update:
https://github.com/openstack/keystone/blob/2764b49bc209245276da31693ef31f1525cf17d8/keystone/catalog/controllers.py#L83

@dklyle dklyle mentioned this pull request Nov 3, 2017
4 tasks
@coveralls
Copy link
Copy Markdown

coveralls commented Nov 3, 2017

Coverage Status

Coverage increased (+0.04%) to 72.258% when pulling 5c96613 on dklyle:region-update into 6f1b7d4 on gophercloud:master.

@dklyle dklyle changed the title Region update Identity v3 Region update Nov 3, 2017
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.

Since there's a bug with updating Extra, perhaps it would be best to leave it out for now?

I think simply commenting the code out with a link to the bug would be fine. This way, when the bug is fixed, the code can be re-enabled without a lot of work.

It'd also be a good idea to add a doc comment that updating Extra will only be valid from the point of the bug being fixed, once it is fixed.

Also added some doc nits since since a larger change is required.

Let me know your thoughts on the above :)


region, err := regions.Create(identityClient, createOpts).Extract()
if err != nil {
panic(err)
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.

missing closing brace

regionID := "TestRegion"
err := regions.Delete(identityClient, regionID).ExtractErr()
if err != nil {
}
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.

missing panic(err)

@coveralls
Copy link
Copy Markdown

coveralls commented Nov 6, 2017

Coverage Status

Coverage decreased (-0.01%) to 72.203% when pulling 4a5288b on dklyle:region-update into 6f1b7d4 on gophercloud:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Nov 6, 2017

Coverage Status

Coverage increased (+0.002%) to 72.203% when pulling 4239f52 on dklyle:region-update into 9c81722 on gophercloud:master.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Nov 7, 2017

Looks good. Thank you, @dklyle!

@jtopjian jtopjian merged commit 6e5b7d6 into gophercloud:master Nov 7, 2017
@dklyle dklyle deleted the region-update branch November 16, 2017 21:30
cardoe pushed a commit to cardoe/gophercloud that referenced this pull request Aug 27, 2020
If "admin_state_up" is not specified in the API request, then the port
should default to an admin_state_up of "UP".

Currently, this resource will always send a value of "false", even if
the user did not specify it in their resource configuration.

This commit fixes this behavior.
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