-
Notifications
You must be signed in to change notification settings - Fork 2
add root_user variable #59
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
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.
This looks fine to me.
There are a couple of comments in the file which still specify "frum" where perhaps they shouldn't now, but functionally this is fine.
Examples at L9, L126, L145
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.
I agree with Erica that this wants a bit more tidying up. I'd suggest the following:
- Move L148 to after L32 to define
root_user=frum
- Move L151 to after L24 to define
root_user="umadmin"
Then do a wholesale search-and-replace to swap frum
and umadmin
for ${root_user}
.
It may also make sense to use this as an opportunity to set another variable in the same initial section between L22 and L33 to define an install directory for each platform, e.g. root_home=/home/users/${root_user}
, and replace any hardwired referenced to the home directory. This future-proofs the script in the event that we decide we need to remove the explicit username references.
Cheers Sam, done those. There's definitely tidying that could be done for the root_home still, but they're not simple copy and pastes so I'll leave for slower time. Any actual refactor would be better left until we've removed old platforms I think. |
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.
Thank you! Changes look good to me.
While reviewing the linter ticket I realised that some commands in the kgo scripts were hardcoded to frum instead of using umadmin as they are on azspice. This ticket fixes that.