Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Use dnf instead of rpm #886

Merged

Conversation

bshephar
Copy link
Contributor

This change updates the hotfix docs to use the dnf command instead of rpm directly

@bshephar
Copy link
Contributor Author

/test precommit-check

@bshephar bshephar force-pushed the dnf-instead-of-rpm branch from d9fc3ff to 913cead Compare May 20, 2024 05:34
@fao89
Copy link
Collaborator

fao89 commented May 20, 2024

/test precommit-check

@@ -34,10 +34,9 @@ Repeat this step for each data plane node that the hotfix must be applied to.
+
----
$ ssh <ssh_user>@<data_plane_node>
$ sudo rpm -F /tmp/<hotfix_id>/rpms/*.rpm
$ sudo dnf in -y /tmp/<hotfix_id>/rpms/*.rpm
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a specific reason why? are you expecting hotfix rpms to need to be able to resolve deps and perhaps install other updates? It seems to me that would be something we would not want to do. I'm pretty sure we have documented using rpm in past releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just aligns with RHEL documented processes for installing and managing packages. AFAIK there's no RHEL recommendations to use rpm commands directly. I think it's probably less confusing for users if we align our recommendations with those used in RHEL documentation.

If there are dependencies, they should probably be installed right? Would we expect that an RPM with dependencies would even work if the dependencies were to be skipped?

The main objective was to align with recommendations for RHEL. But if we feel there's a compelling case to not do that, we can drop this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this rationale for the change is valid. After all, the entire move to asciidocs was performed to harmonize docs across RH products. It makes sense to continue in that direction.
That being said, in the interest of clarity and for posterity, it may be better to explicitly state this in the commit message.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The main objective was to align with recommendations for RHEL. But if we feel there's a compelling case to not do that, we can drop this PR.

I think that explains it, so I'm ok with it. Are there downstream CI considerations that need to be updated as well, or was this all manually tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manually tested with a random RPM. As far as I'm aware, there's currently no downstream jobs that test this process yet.

.github/workflows/docs.yaml Outdated Show resolved Hide resolved
This change updates the hotfix docs to use the dnf command instead of rpm directly

Signed-off-by: Brendan Shephard <[email protected]>
@bshephar bshephar force-pushed the dnf-instead-of-rpm branch from e88888f to 354a6ea Compare May 22, 2024 02:59
@jpodivin jpodivin self-requested a review May 22, 2024 06:58
@jpodivin
Copy link
Contributor

LGTM unless someone has reservations.

Copy link
Contributor

openshift-ci bot commented May 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bshephar, slagle

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit a74da16 into openstack-k8s-operators:main May 22, 2024
7 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants