-
Notifications
You must be signed in to change notification settings - Fork 11
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
add Azure Linux support #87
Conversation
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 for creating the PR. Looks nice.
It is up to you to decide whether to write functional tests in this PR or not.
See below:
@@ -26,10 +27,78 @@ impl Distribution for Distributions { | |||
password: &str, | |||
) -> Result<i32, String> { | |||
match self { | |||
Distributions::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.
As being implemented in the other PR #86, we should not check for each distro, to be flexible and scalable.
I will soon try to update the PR to make it reviewable. Once it got merged, I hope this PR could be rebased and written without the distro checks.
} else { | ||
let input = format!("{}:{}", username, password); | ||
|
||
let mut output = Command::new("chpasswd") |
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.
Sorry, I am not an expert of Azure Linux, but is there a specific reason why you need to do password-based authentication?
Previously we had the password authentication in the code base, but the PR #57 removed that part mainly for security. Though there still seems to be cases where password authentication would be necessary. I am just curious what the situation of Azure Linux would be.
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.
No actually, I believe we do want to eliminate password auth, this was a hold over from me re-implementing the debian logic of an older branch 👍
Thank you for the catch
Hello @SeanDougherty 👋 Thank you for your work, great to see azure-init is useful for you! Just out of curiosity, could you please have a look at #86 and check if that PR "magically" adds Azure-Linux support? We're trying to be distro-agnostic going forward, and it might well be Dongsu's PR will implicitly add support for Azure Linux (and a number of other distros, too). If you need password authentication support for some reason (see Dongsu's question) I'm sure we'll find a way to work this into the generic approach. |
Closing b/c PR is outdated. My new approach will be to work alongside maintainers within PR reviews and GH issues to bring Azure Linux support onboard 👍 |
This PR adds support for Azure Linux to the 0.1.1 version of azure-init. I need to rebase to the current head of main, while I do this, I'm marking the PR as draft to facilitate discussion.
A notable delta between distros is the addition of the "wheel" and "sudo" groups and removal of the other groups in useradd.
Additionally, this PR adds the os-release crate to determine the OS azure-init is operating on during provisioning time.
Finally, Functional tests for AzureLinux are missing. An update of the functional test to support either Azure Linux OR Ubuntu is a goal. I'm happy to make this change to the functional test in this PR or in a separate PR at the maintainer's request.