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.

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

Get instance action fix #1851

Merged
merged 1 commit into from
Feb 19, 2020
Merged

Conversation

snigle
Copy link
Contributor

@snigle snigle commented Feb 14, 2020

For #1845

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

https://docs.openstack.org/api-ref/compute/?expanded=show-server-action-details-detail,list-actions-for-server-detail#show-server-action-details

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 14, 2020

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 14, 2020

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.


// InstanceActionDetail gives more details on instance action.
type InstanceActionDetail struct {
Action string `json:"action"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried with nested type InstanceAction but the func extractIntoPtr didn't work, So I copy paste all field :/

@coveralls
Copy link

coveralls commented Feb 14, 2020

Coverage Status

Coverage increased (+0.01%) to 77.21% when pulling c69d192 on ovh:listInstanceActions into f61d5a0 on gophercloud:master.

@jtopjian
Copy link
Contributor

@snigle What's the purpose of this PR? The title has "fix" in it, but no other details.

Also, instead of API documentation, can you provide the server-side service code links which prove this implementation is correct?

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 14, 2020

Build succeeded.

@snigle
Copy link
Contributor Author

snigle commented Feb 14, 2020 via email

@jtopjian
Copy link
Contributor

@snigle Thanks. It looks like this set of API calls has a number of features added/modified via microversions. The way that some of these features are implemented in this PR aren't following our current guidelines for microversions. For example, there should be dedicated extract methods for Host and HostID.

However, I'm currently refactoring things to simplify our guidelines (#1854). Once this is merged, you'll have to make some minor changes, which I'll outline in a review.

@snigle
Copy link
Contributor Author

snigle commented Feb 17, 2020

@snigle Thanks. It looks like this set of API calls has a number of features added/modified via microversions. The way that some of these features are implemented in this PR aren't following our current guidelines for microversions. For example, there should be dedicated extract methods for Host and HostID.

However, I'm currently refactoring things to simplify our guidelines (#1854). Once this is merged, you'll have to make some minor changes, which I'll outline in a review.

Ok I updated the code as the same way of your PR.
b540030

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 17, 2020

Build succeeded.

@jtopjian
Copy link
Contributor

@snigle Nice - thank you! Everything looks correct. The only remaining item is fixing the Travis errors:

https://travis-ci.org/gophercloud/gophercloud/jobs/651404710#L492-L495

Because some fields were changed to pointers, some of the fixtures need modified, too. For example:

https://github.com/gophercloud/gophercloud/pull/1851/files#diff-a5d110494cfe4ed97e9f81b89d0e752eR76

var updatedAt = time.Date(2018, 04, 25, 1, 26, 36, 0, time.UTC)
var GetExpected = instanceactions.InstanceActionDetail{
  ...
  UpdatedAt: &updatedAt,
}

@snigle
Copy link
Contributor Author

snigle commented Feb 18, 2020

@snigle Nice - thank you! Everything looks correct. The only remaining item is fixing the Travis errors:

https://travis-ci.org/gophercloud/gophercloud/jobs/651404710#L492-L495

Because some fields were changed to pointers, some of the fixtures need modified, too. For example:

https://github.com/gophercloud/gophercloud/pull/1851/files#diff-a5d110494cfe4ed97e9f81b89d0e752eR76

var updatedAt = time.Date(2018, 04, 25, 1, 26, 36, 0, time.UTC)
var GetExpected = instanceactions.InstanceActionDetail{
  ...
  UpdatedAt: &updatedAt,
}

Oups .... Forgotten to run test locally :s Fixed now :)

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 18, 2020

Build succeeded.

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.

LGTM - thank you for your work and patience with this one!

@jtopjian jtopjian merged commit 14f04f5 into gophercloud:master Feb 19, 2020
@Benzhaomin Benzhaomin deleted the listInstanceActions branch November 2, 2021 14:10
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.

None yet

3 participants