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

redfish_utils: Let @Redfish.OperationApplyTime be optional #8824

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cwhuang
Copy link

@cwhuang cwhuang commented Sep 3, 2024

As the doc states, this is not a required option. AMI BMC will complain since it's not supported. So don't add this option unless it's explicitly specified by update_apply_time.

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

plugins/module_utils/redfish_utils.py

ADDITIONAL INFORMATION

As the doc states, this is not a required option. AMI BMC will complain
since it's not supported. So don't add this option unless it's explicitly
specified by update_apply_time.

Signed-off-by: Chih-Wei Huang <[email protected]>
@ansibullbot
Copy link
Collaborator

@cwhuang This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module_utils module_utils needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Sep 3, 2024
AMI BMC requires a separate OemParameters json to set the image type
when doing MultipartHTTPPushUpdate.

The patch adds a new option update_image_type to support it.
Test OK with AMI BMC 13.3.

Signed-off-by: Chih-Wei Huang <[email protected]>
@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added module module and removed small_patch Hopefully easy to review labels Sep 3, 2024
@@ -1899,6 +1900,9 @@ def multipath_http_push_update(self, update_opts):
'UpdateParameters': {'content': json.dumps(payload), 'mime_type': 'application/json'},
'UpdateFile': {'filename': image_file, 'content': image_payload, 'mime_type': 'application/octet-stream'}
}
if image_type:
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no "image type" defined in the Redfish Specification. This looks like a very specific OEM definition for the OEM multipart form.

Would it be possible to make this more generic to accommodate other OEM usage? There's no predefined data type (it could be XML, JSON, binary, plain text...), and the form name isn't specifically "OemParameters".

Please check out Table 40 — Multipart HTTP push updates in the Redfish Spec here: https://www.dmtf.org/sites/default/files/standards/documents/DSP0266_1.20.2.pdf

Copy link
Author

Choose a reason for hiding this comment

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

Actually the pull request should only contain the first commit.
It just removes the default value set for @Redfish.OperationApplyTime.

The second commit is for another feature request I created in issue #8825
I planned to send it in another pull request. But seems github combined them
automatically probably because I put them in the same branch.
Sorry for the confusion since I'm not familiar enough with the process.
Let me know how to do it correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can do git revert <COMMITHASH> to back out your changes from a given commit.

You then should make a new branch and cherry-pick that commit over.

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-8 Automatically create a backport for the stable-8 branch backport-9 Automatically create a backport for the stable-9 branch labels Sep 4, 2024
@felixfontein
Copy link
Collaborator

Thanks for your contribution! Please note that you need to add a changelog fragment. Thanks.

@felixfontein felixfontein removed the backport-8 Automatically create a backport for the stable-8 branch label Sep 4, 2024
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Sep 12, 2024
@felixfontein
Copy link
Collaborator

@cwhuang ping

needs_info

@ansibullbot ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Oct 7, 2024
@felixfontein felixfontein removed the backport-9 Automatically create a backport for the stable-9 branch label Oct 7, 2024
@ansibullbot
Copy link
Collaborator

@cwhuang This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.

click here for bot help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug check-before-release PR will be looked at again shortly before release and merged if possible. module_utils module_utils module module needs_info This issue requires further information. Please answer any outstanding questions needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants