Skip to content

Baremetal virtual media Get API#3303

Merged
mandre merged 3 commits intogophercloud:mainfrom
hroyrh:vmedia_get
Mar 11, 2025
Merged

Baremetal virtual media Get API#3303
mandre merged 3 commits intogophercloud:mainfrom
hroyrh:vmedia_get

Conversation

@hroyrh
Copy link
Copy Markdown
Contributor

@hroyrh hroyrh commented Feb 17, 2025

Fixes #3302

@github-actions github-actions bot added the edit:baremetal This PR updates baremetal code label Feb 17, 2025
github-actions[bot]
github-actions bot reviewed Feb 17, 2025
View reviewed changes
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for submitting your first PR! Be sure that we will be looking at it but keep in mind
this sometimes takes a while.
Please let the maintainers know if your PR has not got enough attention after a few days.
If any doubt, please consult our PR tutorial.

@github-actions github-actions bot added the semver:minor Backwards-compatible change label Feb 17, 2025
@coveralls
Copy link
Copy Markdown

coveralls commented Feb 17, 2025 •
edited
Loading

Coverage Status

coverage: 78.686% (+0.007%) from 78.679%
when pulling 6751a7a on hroyrh:vmedia_get
into 5afeaf8 on gophercloud:main.

@Sharpz7
Copy link
Copy Markdown
Contributor

Sharpz7 commented Feb 19, 2025

This looks like a microversion change that is needed. Let me know if you want help implementing that fix

Sharpz7
Sharpz7 reviewed Feb 19, 2025
View reviewed changes
internal/acceptance/openstack/baremetal/v1/nodes_test.go
client, err := clients.NewBareMetalV1Client()
th.AssertNoErr(t, err)
client.Microversion = "1.89"
client.Microversion = "1.93"
Copy link
Copy Markdown
Contributor

@Sharpz7 Sharpz7 Feb 19, 2025

Choose a reason for hiding this comment

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

Although, it does look like you have updated it, so my confidence on that is low.

Copy link
Copy Markdown
Contributor Author

@hroyrh hroyrh Feb 21, 2025

Choose a reason for hiding this comment

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

Yes, the issue is driver related, the test is using ipmi but the feature only supports redfish. I'm looking into fixing it.

@iurygregory
Copy link
Copy Markdown
Contributor

iurygregory commented Mar 3, 2025

the tests are failing, but they are legit failures. get api will only work on redfish based.. so you can remove the change in the acceptance, I will be trying to move things to redfish in another PR and later we can update I would say.

I've manually tested this with ironic directly and things should work, I'm just wondering what we expect as the output when doing

Via curl we would get something like:

[
  {
    "media_types": [
      "Floppy",
      "USBStick"
    ],
    "inserted": false,
    "image": ""
  },
  {
    "media_types": [
      "CD",
      "DVD"
    ],
    "inserted": true,
    "image": "http://<address>/redfish/boot-12345678-aaaa-5856-bfb6-6aaaacd3dd11.iso"
  }
]

With gophercloud doing

result := nodes.GetVirtualMedia(ctx, client, myID)

returns the following:

{{[map[image: inserted:false media_types:[Floppy USBStick]] map[image:http://<address>/redfish/boot-12345678-aaaa-5856-bfb6-6aaaacd3dd11.iso inserted:true media_types:[CD DVD]]] 0 map[Content-Length:[212] Content-Type:[application/json] Date:[Fri, 28 Feb 2025 01:26:50 GMT] Openstack-Request-Id:[req-...] X-Openstack-Ironic-Api-Maximum-Version:[1.95] X-Openstack-Ironic-Api-Minimum-Version:[1.1] X-Openstack-Ironic-Api-Version:[1.93]] <nil>}}

using Result

result := nodes.GetVirtualMedia(ctx, client, myID).Result

{[map[image: inserted:false media_types:[Floppy USBStick]] map[image:http://<address>/redfish/boot-12345678-aaaa-5856-bfb6-6aaaacd3dd11.iso inserted:true media_types:[CD DVD]]] 0 map[Content-Length:[212] Content-Type:[application/json] Date:[Fri, 28 Feb 2025 01:31:18 GMT] Openstack-Request-Id:[req-34c36302-b93f-4492-89a8-c1e27f3a8cbb] X-Openstack-Ironic-Api-Maximum-Version:[1.95] X-Openstack-Ironic-Api-Minimum-Version:[1.1] X-Openstack-Ironic-Api-Version:[1.93]] <nil>}

using Err

result := nodes.GetVirtualMedia(ctx, client, myID).Err

<nil>

using Body

result := nodes.GetVirtualMedia(ctx, client, myID).Body

[map[image: inserted:false media_types:[Floppy USBStick]] map[image:http://<address>/redfish/boot-12345678-aaaa-5856-bfb6-6aaaacd3dd11.iso inserted:true media_types:[CD DVD]]]

@github-actions github-actions bot removed the semver:minor Backwards-compatible change label Mar 7, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 7, 2025

Failed to assess the semver bump. See logs for details.

@github-actions github-actions bot added the semver:minor Backwards-compatible change label Mar 7, 2025
@iurygregory
Copy link
Copy Markdown
Contributor

iurygregory commented Mar 7, 2025

LGTM

@iurygregory
Copy link
Copy Markdown
Contributor

iurygregory commented Mar 7, 2025

@EmilienM when you have time can you check this PR? Thanks!

@hroyrh
ignore error for vmedia get api for ipmi driver
6751a7a 
Signed-off-by: Himanshu Roy <hroy@redhat.com>
@mandre mandre merged commit 1031a87 into gophercloud:main Mar 11, 2025
18 checks passed
@hroyrh hroyrh mentioned this pull request Mar 21, 2025
Merged
@pierreprinetti
Copy link
Copy Markdown
Member

pierreprinetti commented Mar 23, 2025

@mandre Why was this not backported to v2?

@mandre
Copy link
Copy Markdown
Contributor

mandre commented Mar 23, 2025

@mandre Why was this not backported to v2?

It was an oversight.

@mandre mandre added the backport-v2 This PR will be backported to v2 label Mar 23, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 23, 2025

Failed to backport PR to v2 branch. See logs for details.

@mandre
Copy link
Copy Markdown
Contributor

mandre commented Mar 23, 2025

The backport to v2 depends on #3334.

@mandre mandre added backport-v2 This PR will be backported to v2 and removed backport-v2 This PR will be backported to v2 labels Mar 23, 2025
@gophercloud-backport-bot gophercloud-backport-bot bot mentioned this pull request Mar 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@mandre mandre mandre approved these changes

+1 more reviewer

@Sharpz7 Sharpz7 Sharpz7 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport-v2 This PR will be backported to v2 edit:baremetal This PR updates baremetal code semver:minor Backwards-compatible change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Baremetal: add support for new virtual media GET API

6 participants

@hroyrh @coveralls @Sharpz7 @iurygregory @pierreprinetti @mandre