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

Added ability to create OS users #44

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Added ability to create OS users #44

merged 3 commits into from
Nov 17, 2023

Conversation

jdob
Copy link
Contributor

@jdob jdob commented Nov 16, 2023

This PR does not include checks to ensure at least one of password/SSH key are specified per user. We likely want that (I can't think of a use case for manually creating a user that can't be logged in that we'd want to support here), but that'll wait until the larger validation infra is in place for the entire image definition.

@jdob jdob requested review from atanasdinov and dbw7 November 16, 2023 20:55
Copy link
Contributor

@atanasdinov atanasdinov left a comment

Choose a reason for hiding this comment

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

Looking great! Just a comment on the tests setup that I'd like to bring up.

pkg/config/image_test.go Outdated Show resolved Hide resolved
{{ end }}

{{/* Root */}}
{{ if (eq .Username "root") }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason not to {{ else }} instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing specific. My head was in a different place editing a template v. code so I just wasn't thinking of it as a if/else construct so much as a flag for inclusion (not sure if that made sense).

pkg/build/users_test.go Outdated Show resolved Hide resolved
Comment on lines +13 to +15
{{ if .Password }}
echo '{{.Username}}:{{.Password}}' | chpasswd -e
{{ end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the same for both cases can we extract it down below in order to avoid the repetition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO that makes it less readable, but that may be a personal distaste for the templating syntax. I like the model here where we have distinct section on root v. non-root and, IMO, that's worth the minimal duplication.

@jdob jdob merged commit 3962567 into suse-edge:main Nov 17, 2023
2 checks passed
@jdob jdob deleted the users branch January 10, 2024 16:18
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.

2 participants