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

Update update.precheck/apply to be compatible with yum and dnf #5564

Merged
merged 5 commits into from
Apr 18, 2024

Conversation

liulinC
Copy link
Collaborator

@liulinC liulinC commented Apr 17, 2024

Testing done:

  • Some manual test for dnf install, xe update-apply
  • HfxBasic: 3981563
  • SuppPackBasic: 3981564 (one failed with unrelated error)

Given XS9 has deplicated yum with dnf, to be compatible with XS8,
- Use dnf if dnf is detected
- Fallback to yum otherwise

Signed-off-by: Lin Liu <lin.liu@citrix.com>
Signed-off-by: Lin Liu <lin.liu@citrix.com>
@liulinC liulinC changed the title Update update precheck/apply to be compatible with yum and dnf Update update.precheck/apply to be compatible with yum and dnf Apr 17, 2024
@liulinC
Copy link
Collaborator Author

liulinC commented Apr 17, 2024

This PR contains two commits

  • first one, real functional update
  • second one, just some updates to make CI happy (pylint/pycheck).

scripts/extensions/pool_update.precheck Outdated Show resolved Hide resolved
scripts/extensions/pool_update.precheck Outdated Show resolved Hide resolved
scripts/extensions/pool_update.precheck Outdated Show resolved Hide resolved
@@ -48,6 +48,14 @@ ERRORCODE = 'errorcode'
ERROR = 'error'
FOUND = 'found'
REQUIRED = 'required'
YUM_CMD = '/usr/bin/yum'
DNF_CMD = '/usr/bin/dnf'
PKG_MGR = DNF_CMD if os.path.exists(DNF_CMD) else YUM_CMD
Copy link
Contributor

@lindig lindig Apr 17, 2024

Choose a reason for hiding this comment

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

This is not using OO as it could: have a class that does what we need and zwo small sub-classes: one for yum and one for dnf. This would avoid the branching we are seeing here. It's probably ok because I don't expect more variation here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it can be.
As you already point out, this is only tiny diff here between yum and dnf.
Even with class, we need branching which class to use anyway, so I prefer to just this none-OO styple, does this make sense? (If more and more variation comes, then we can use OO later.)

liulinC and others added 2 commits April 17, 2024 17:42
Co-authored-by: Luca Zhang <feiya.zhang@cloud.com>
Signed-off-by: liulinC <lin.liu@citrix.com>
Co-authored-by: Luca Zhang <feiya.zhang@cloud.com>
Signed-off-by: liulinC <lin.liu@citrix.com>
@liulinC
Copy link
Collaborator Author

liulinC commented Apr 17, 2024

@duobei Thanks for the help. I was intentionally to keep the original code to

  • de-risk this updates
  • Do not miss up new update and technical debt
  • Make reviewers life easy. (and highlight the functional change in this PR)
    But given you have point that out, I will apply the suggestions.

Signed-off-by: Lin Liu <lin.liu@citrix.com>
@liulinC liulinC requested review from lindig and duobei April 17, 2024 09:55
@duobei
Copy link
Contributor

duobei commented Apr 17, 2024

@duobei Thanks for the help. I was intentionally to keep the original code to

  • de-risk this updates
  • Do not miss up new update and technical debt
  • Make reviewers life easy. (and highlight the functional change in this PR)
    But given you have point that out, I will apply the suggestions.

I'm sorry for the trouble. I'll take care of these next.

@liulinC
Copy link
Collaborator Author

liulinC commented Apr 18, 2024

@duobei Thanks for the help. I was intentionally to keep the original code to

  • de-risk this updates
  • Do not miss up new update and technical debt
  • Make reviewers life easy. (and highlight the functional change in this PR)
    But given you have point that out, I will apply the suggestions.

I'm sorry for the trouble. I'll take care of these next.

All the suggestions are really good point, thanks again for the help. 👍

@liulinC liulinC merged commit ca8ddc4 into xapi-project:feature/xs9 Apr 18, 2024
13 checks passed
Copy link

pytype_reporter extracted 50 problem reports from pytype output

.

You can check the results of the job here

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