-
Notifications
You must be signed in to change notification settings - Fork 884
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
feat(azurelinux): Add new distro 'azurelinux' for Microsoft Azure Linux. #4931
Conversation
cloudinit/config/cc_ntp.py
Outdated
@@ -25,6 +25,7 @@ | |||
distros = [ | |||
"almalinux", | |||
"alpine", | |||
"azure", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
azurelinux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! fixed
cloudinit/distros/azurelinux.py
Outdated
# For example: if you request to install foo, bar and baz and baz | ||
# is installed; yum won't error out complaining that baz is already | ||
# installed. | ||
cmd = ["yum", "-t"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider deprecating yum. Default to tdnf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yum
is only being used as a final fallback here, after attempting dnf
and tdnf
first...we could switch the final fallback over to tdnf
instead (and drop falling back to yum
completely) if that's what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is what I meant. I was thinking we should switch the final fallback to tdnf
and drop yum
, since yum
for us is really a symlink to tdnf
, and we ought to remove this in our upcoming major release - https://github.com/microsoft/CBL-Mariner/blob/3.0-dev/SPECS/tdnf/tdnf.spec#L147
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, updated, thanks!
710c1b9
to
9acc310
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
d4c3591
to
b0dc55e
Compare
rebased |
updated |
@ddstreetmicrosoft Thanks for this PR. Does this mean that the Mariner datasource is no longer relevant? Should we remove it at some point? If you check the CI runs, you'll see there's a few lint failures that need to be fixed before we can merge this. Additionally, before we can accept your change, you need to sign the CLA and add your Github username (alphabetically) to the cla signers file . The full details are described in the last bullet point of the documentation . It looks like you may have already signed the CLA under a different github name, so if you're using two ( |
LGTM. Looks like |
While the Alternately, I could open a separate PR to remove 'Mariner', either soon or closer to when it goes EOL (which will be late this year at the earliest, more likely sometime next year).
ack, thanks, i ran
i signed the CLA and included both my github usernames ( |
Yes, a separate PR sounds good. If there is truly no need for the upstream code anymore, then soon is fine, but there is also no harm in keeping it here for the time being. |
The 3.13 test failure is unrelated and being addressed in #5052 . If everything else passes I'll merge as-is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @ddstreet
Proposed Commit Message
Additional Context
Checklist
Merge type