Skip to content

Baremetal API: Update CleanStep struct to support manual cleaning#1638

Merged
jtopjian merged 1 commit intogophercloud:masterfrom
longkb:update_cleansteps_struct
Jul 4, 2019
Merged

Baremetal API: Update CleanStep struct to support manual cleaning#1638
jtopjian merged 1 commit intogophercloud:masterfrom
longkb:update_cleansteps_struct

Conversation

@longkb
Copy link
Copy Markdown
Contributor

@longkb longkb commented Jul 4, 2019 •
edited
Loading

Currently, the Args property in CleanStep struct has **map[string]string that
does not match with manual cleaning steps in Ironic. It should be dictionary in python[1][2] and interface{} in golang . For example:

BIOS setting for a node:

[
        {
            "interface": "bios",
            "step": "apply_configuration",
            "args": {
                "settings": [{"name": "hyper_threading_enabled", "value": "false"}]
            }
        }
]

This PR aims to update Args in CleanStep struct.
[1] https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L76
[2] https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L624

Signed-off-by: Kim Bao Long longkb@vn.fujitsu.com

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 4, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 76.859% when pulling d2c7784 on longkb:update_cleansteps_struct into 2949719 on gophercloud:master.

@Yikun
Copy link
Copy Markdown

Yikun commented Jul 4, 2019

recheck

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jul 4, 2019

Build succeeded.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jul 4, 2019

@longkb Thank you for submitting this. Would you be able to read over our contributor tutorial?

Notably, can you link to the Ironic server-side code that allows this to be a map[string]interface{} instead of a map[string]string? And would you be able to add some unit tests to validate this change?

Please let me know if you have any questions or need any help.

@longkb
Copy link
Copy Markdown
Contributor Author

longkb commented Jul 4, 2019 •
edited
Loading

@jtopjian : Thanks for your comment.
I have update the commit message with the reference to ironic side. From _CLEAN_STEPS_SCHEMA [1], args is declare as object type, and it is also metioned as dictionary instead on string in description[2].

[1] https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L76
[2] https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L624

@longkb
Update CleanStep struct to support manual cleaning
d2c7784 
Currently, the **Args** property in CleanStep struct has **map[string]string that
does not match with manual cleaning steps in Ironic. It should be dictionary in
python[1][2] and interface{} in golang. For example:

BIOS setting for a node:
```
[
        {
            "interface": "bios",
            "step": "apply_configuration",
            "args": {
                "settings": [{"name": "hyper_threading_enabled", "value": "false"}]
            }
        }
]
```
This PR aims to update **Args** in **CleanStep** struct.
[1] https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L76
[2] https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py#L624

Signed-off-by: Kim Bao Long <longkb@vn.fujitsu.com>
@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jul 4, 2019

From _CLEAN_STEPS_SCHEMA [1], args is declare as object type, and it is also metioned as dictionary instead on string in description[2].

That's perfect - thank you!

Technically, we should also update the unit tests to account for this change, too. The unit tests are valid, but it would be better to use a more detailed map[string]interface{}. I'm okay with leaving things as they are. This comment is more for posterity in case this topic comes up again.

Thank you for your work on this.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jul 4, 2019

recheck

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jul 4, 2019

Build succeeded.

@jtopjian jtopjian merged commit 85ccb46 into gophercloud:master Jul 4, 2019
@longkb
Copy link
Copy Markdown
Contributor Author

longkb commented Jul 5, 2019

Wow, it's merged! 👍
Thanks @jtopjian :)
I am a new comer in Gopher Cloud, so your comment is very useful for me. I hope that I will have more contribution in Gopher Cloud :)
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@longkb @coveralls @Yikun @jtopjian