Skip to content

Get instance action fix#1851

Merged
jtopjian merged 1 commit intogophercloud:masterfrom
ovh:listInstanceActions
Feb 19, 2020
Merged

Get instance action fix#1851
jtopjian merged 1 commit intogophercloud:masterfrom
ovh:listInstanceActions

Conversation

@snigle
Copy link
Copy Markdown
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
Copy Markdown

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.

@snigle snigle force-pushed the listInstanceActions branch from 91020b3 to e912397 Compare February 14, 2020 13:47
@theopenlab-ci
Copy link
Copy Markdown

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.

@snigle snigle force-pushed the listInstanceActions branch from e912397 to 3b4dab9 Compare February 14, 2020 13:48

// InstanceActionDetail gives more details on instance action.
type InstanceActionDetail struct {
Action string `json:"action"`
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

theopenlab-ci bot commented Feb 14, 2020

Build succeeded.

@snigle
Copy link
Copy Markdown
Contributor Author

snigle commented Feb 14, 2020 via email

@jtopjian
Copy link
Copy Markdown
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
Copy Markdown
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

@snigle snigle force-pushed the listInstanceActions branch from 016a7d6 to dd9aec2 Compare February 17, 2020 09:30
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 17, 2020

Build succeeded.

@jtopjian
Copy link
Copy Markdown
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 snigle force-pushed the listInstanceActions branch from 95c9dbb to c69d192 Compare February 18, 2020 08:18
@snigle
Copy link
Copy Markdown
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
Copy Markdown

theopenlab-ci bot commented Feb 18, 2020

Build succeeded.

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 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.

3 participants