Skip to content

schedulerhints: support DifferentCell filter#2012

Merged
jtopjian merged 1 commit intogophercloud:masterfrom
cardoe:add-different-cell
Aug 27, 2020
Merged

schedulerhints: support DifferentCell filter#2012
jtopjian merged 1 commit intogophercloud:masterfrom
cardoe:add-different-cell

Conversation

@cardoe
Copy link
Copy Markdown
Contributor

@cardoe cardoe commented Aug 27, 2020

The TargetCell filter from cells v1 is supported in the code base but
support for the DifferentCell filter was never added. DifferentCell
filter is part of cells v1 but for anyone still using cells v1 this is
useful.

For #2011

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

openstack/nova@596ee87

@cardoe
Copy link
Copy Markdown
Contributor Author

cardoe commented Aug 27, 2020

The tests fail because of the identity/v3 module uses the internal module which isn't allowed. My change doesn't involve identity at all so the failures seemed out of scope to me.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Aug 27, 2020

@cardoe Thank you for submitting this.

The tests fail because of the identity/v3 module uses the internal module which isn't allowed.

I'm not sure what you mean. Can you elaborate?

I believe the unit tests are failing because of a syntax error: https://travis-ci.org/github/gophercloud/gophercloud/jobs/721697827#L286

In addition, it looks like go fmt will need to be run on these changed files. You can see how the indentation isn't matching up in the diff.

Please let me know if you have any questions.

@cardoe cardoe force-pushed the add-different-cell branch 3 times, most recently from 2de6199 to 9e98fcb Compare August 27, 2020 14:29
@cardoe
Copy link
Copy Markdown
Contributor Author

cardoe commented Aug 27, 2020

Ah I was looking at https://travis-ci.com/github/cardoe/gophercloud/jobs/378393317

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Aug 27, 2020

How strange - especially because the latest run didn't trigger that error.

It's looking like more go fmt errors. Did you run the actual go fmt tool or just change the indentation? If the former, then this is some wonky behavior with go fmt between releases and I'll add it to an ignore list we have.

@cardoe
schedulerhints: support DifferentCell filter
789d07e 
The TargetCell filter from cells v1 is supported in the code base but
support for the DifferentCell filter was never added. DifferentCell
filter is part of cells v1 but for anyone still using cells v1 this is
useful. Originally part of OpenStack with openstack/nova@596ee87
@cardoe cardoe force-pushed the add-different-cell branch from 9e98fcb to 789d07e Compare August 27, 2020 14:49
@coveralls
Copy link
Copy Markdown

coveralls commented Aug 27, 2020

Coverage Status

Coverage increased (+0.002%) to 79.514% when pulling 789d07e on cardoe:add-different-cell into 77b238f on gophercloud:master.

@cardoe
Copy link
Copy Markdown
Contributor Author

cardoe commented Aug 27, 2020

@jtopjian I've just made the changes to make it happy. A real rough test with this in the terraform-provider-openstack is all I've done. I'm ultimately looking to make a PR there to make this work.

@cardoe
Copy link
Copy Markdown
Contributor Author

cardoe commented Aug 27, 2020

https://github.com/cardoe/terraform-provider-openstack/tree/different-cell-scheduler ultimately this is what I'd like to land.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Aug 27, 2020

Build succeeded.

jtopjian
jtopjian approved these changes Aug 27, 2020
View reviewed changes
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 bb4781e into gophercloud:master Aug 27, 2020
@jtopjian jtopjian mentioned this pull request Aug 27, 2020
Closed
@cardoe cardoe deleted the add-different-cell branch September 21, 2020 13:44
@cardoe cardoe restored the add-different-cell branch June 22, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtopjian jtopjian jtopjian approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cardoe @jtopjian @coveralls