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.

Sign up for GitHub

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

Jump to bottom

Compute v2: Delete aggregate #740

Merged
merged 3 commits into from
Jan 30, 2018
Merged

Compute v2: Delete aggregate #740

merged 3 commits into from
Jan 30, 2018

Conversation

dstdfx
Copy link
Contributor

@dstdfx dstdfx commented Jan 27, 2018

For #738

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

API doc:

Source code:

@dstdfx dstdfx mentioned this pull request Jan 27, 2018
Merged
@coveralls
Copy link

coveralls commented Jan 27, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 73.279% when pulling f6ef5e4 on dstdfx:aggregates-delete into a924af7 on gophercloud:master.

@dstdfx dstdfx mentioned this pull request Jan 27, 2018
Closed
7 tasks
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 27, 2018

Build succeeded.

@jtopjian
Copy link
Contributor

@dstdfx This will need rebased. Looks good to me, otherwise.

@dstdfx
Copy link
Contributor Author

dstdfx commented Jan 29, 2018

@jtopjian Done :)

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 29, 2018

Build succeeded.

@jtopjian
Copy link
Contributor

@dstdfx I did a quick test of this and it looks like Nova is returning a 200 code. Can you confirm if that's what you're seeing?

=== RUN   TestAggregatesDelete
--- FAIL: TestAggregatesDelete (3.10s)
        aggregates_test.go:21: Expected HTTP response code [202 204] when accessing [DELETE http://localhost:8774/v2.1/6c3c1bb5e47a4c5982b8a87f61c62027/os-aggregates/1], but got 200 instead

@dstdfx
Copy link
Contributor Author

dstdfx commented Jan 29, 2018

@jtopjian You're right. I've changed it.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 29, 2018

Build succeeded.

jtopjian
jtopjian requested changes Jan 30, 2018
View reviewed changes
Copy link
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.

@dstdfx Just one more minor change and this is good to go.

openstack/compute/v2/extensions/aggregates/requests.go Outdated
func Delete(client *gophercloud.ServiceClient, aggregateID int) (r DeleteResult) {
v := strconv.Itoa(aggregateID)
_, r.Err = client.Delete(aggregatesDeleteURL(client, v), &gophercloud.RequestOpts{
OkCodes: []int{200}})
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very minor nit and I apologize, but we should make this look like the other OkCodes:

_, r.Err = client.Delete(aggregatesDeleteURL(client, v), &gophercloud.RequestOpts{
        OkCodes: []int{200},
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, I'm surprised that go fmt didn't fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was surprised that both fmt and vet didn't flag it. Thank you for fixing this :)

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 30, 2018

Build succeeded.

@jtopjian
Copy link
Contributor

@dstdfx Thank you for your work on this. If you can, can you submit a PR with an acceptance test for this?

@jtopjian jtopjian merged commit 8128df2 into gophercloud:master Jan 30, 2018
@dstdfx
Copy link
Contributor Author

dstdfx commented Jan 30, 2018

@jtopjian Sure, next PR will be an acceptance test for create/delete aggregates.

@dstdfx dstdfx deleted the aggregates-delete branch January 30, 2018 20:14
dstdfx added a commit to dstdfx/gophercloud that referenced this pull request Jan 30, 2018
@dstdfx
Add acceptance test for create and delete aggregate gophercloud#739, g…
3c56f1b 
…ophercloud#740
@dstdfx dstdfx mentioned this pull request Jan 30, 2018
Merged
dstdfx added a commit to dstdfx/gophercloud that referenced this pull request Jan 31, 2018
@dstdfx
Add acceptance test for create and delete aggregate gophercloud#739, g…
45de072 
…ophercloud#740
jtopjian pushed a commit that referenced this pull request Feb 2, 2018
@dstdfx @jtopjian
Acceptance test for create, get and delete aggregate (#745)
1861349 
* Add acceptance test for create and delete aggregate #739, #740

* Add reusable functions for Create/Delete aggregates

* Fix acceptance test
cardoe pushed a commit to cardoe/gophercloud that referenced this pull request Aug 27, 2020
@jtopjian
Support acceptance test against OpenStack Stein release in OpenLab (g…
8ff1b18 
…ophercloud#740)

Input comment "recheck stable/stein" in pull request comments, that
will trigger acceptance tests against OpenStack Stein release. OpenLab
will report test result in followint comments automatically.

Related: theopenlab/openlab#231
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.

None yet
3 participants
@dstdfx @coveralls @jtopjian