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

Set hostname #113

Merged
merged 23 commits into from Aug 20, 2021
Merged

Set hostname #113

merged 23 commits into from Aug 20, 2021

Conversation

meisenzahl
Copy link
Member

Bildschirmfoto von 2021-08-07 21 21 07

Peek.2021-08-07.21-22.mp4

@cassidyjames I hope this is what you had in mind?

@cassidyjames
Copy link
Contributor

Hmmm. I know a device name is not technically the same as the user, but I wonder if we could include it on that view since it’s just a single field?

@meisenzahl
Copy link
Member Author

Bildschirmfoto von 2021-08-08 10 49 10

Hmm, I don't really like it because the field doesn't fit the title of the page thematically.

@cassidyjames
Copy link
Contributor

cassidyjames commented Aug 8, 2021

Thoughts from @elementary/ux?

My worry is adding a whole extra page and step for something that most people probably should just leave with the defaults (since the installer chooses a hardware-based name with a unique hash). It feels really heavy to say, "here's a whole step, you probably don't want or need to change it, but here you go!" whereas it feels a bit more natural to include the single field along with the first user's details.

The fact that the device name is not technically part of the account is probably fine?

@danirabbit
Copy link
Member

Maybe we can double space between the rest of the user details and the device name? It does feel a little bit awkward since it's kinda unrelated to creating the user

@vjr
Copy link
Member

vjr commented Aug 9, 2021

How about not doing this in initial setup and instead making the hostname editable in the about plug and/or sharing plug? This will allow people to change the name later on as well if they feel the need.

The default can be left as elementary-os initially...

See elementary/switchboard-plug-sharing#53

@meisenzahl
Copy link
Member Author

With increased spacing to hostname:

Bildschirmfoto von 2021-08-09 06 53 22

@danirabbit danirabbit added this to In progress in 6.0 Odin Release via automation Aug 9, 2021
@danirabbit
Copy link
Member

Design looks good, but I can't confirm that this is working. Is there any validation that we need to be doing here? Like symbols that aren't allowed, spaces etc?

@danirabbit danirabbit removed this from In progress in 6.0 Odin Release Aug 9, 2021
@danirabbit danirabbit added this to In progress in OS 6.1 via automation Aug 9, 2021
@meisenzahl
Copy link
Member Author

@danrabbit I should have read the documentation first:

SetStaticHostname sets the device name.

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

@meisenzahl it looks like according to https://www.freedesktop.org/software/systemd/man/org.freedesktop.hostname1.html we need to use SetPrettyHostname as well and we should be transforming the value of SetStaticHostname the same way we do username. So for example:

  • The user inputs "Daniel's Yoga"
  • SetPrettyHostname should use the value Daniel's Yoga
  • SetStaticHostname should use the value daniels-yoga

@meisenzahl
Copy link
Member Author

@danrabbit thanks for reviewing so accurately despite your stress with the release.

I hope that I have now taken everything into account.

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

Couple of other small comments. It also looks like there's a permissions issue. Adding the hostname interface to the pkla file in data seems to fix it

src/Utils.vala Outdated Show resolved Hide resolved
src/Views/AccountView.vala Outdated Show resolved Hide resolved
src/Views/AccountView.vala Outdated Show resolved Hide resolved
@meisenzahl
Copy link
Member Author

The device name is now invalid if it is prefilled with the current hostname:

image

@danirabbit
Copy link
Member

Hm I'm not sure we need to force people to change the name. It's possible that they're perfectly happy with the generated name

@iancleary
Copy link

This is something I did miss during Elementary OS 6 installation today and noticed was absent (relative to the Ubuntu 21.04 installer).

Certain users or organizations prefer to have a hostname naming convention for their machines (lp-icleary-eos, framework-laptop etc.). That may be handled in their processes after the OEM installation step (I'm not that close to that world, but just brainstorming if this option is useful to those folks).

I was able to edit /etc/hostname, /etc/hosts/ as normal to change it to my preference. I'm now just realizing that method may be ignorant of the pretty hostname concept (today I learned 😄 ).

I think giving users the option during the installer flow would be a clean improvement, especially to align with the paradigm of not needing the command line (while it being available if you need it). I say this assuming the above design considerations are met within the Users screen to make it less awkward and not knowing how many people change the generated name. So not a big deal for me to do it post install, but I just wanted to share my user experience.


Congrats on the release! 🥳 I'm excited to test it out on my Framework laptop!
Thanks again for all you do with elementary OS.

@meisenzahl
Copy link
Member Author

@danrabbit sorry, I should have worded that differently. My problem is that by default the entry for the hostname is invalid. This is not intentional on my part, but I currently can't find a solution that validates the hostname properly when programmatically setting it:

hostname_entry = new Granite.ValidatedEntry () {
    activates_default = true,
    hexpand = true,
    text = Utils.get_hostname ()
};

I would appreciate some help 😃

@meisenzahl
Copy link
Member Author

@danrabbit I think I got it right.

src/Utils.vala Outdated Show resolved Hide resolved
@danirabbit
Copy link
Member

@meisenzahl I can confirm validation is working now. We're stilling running into some permission errors it looks like. I've tried updating the pkla file here but for some reason Polkit.Permission.sync is not returning with permission

Co-authored-by: Cassidy James Blaede <cassidy@elementary.io>
Copy link
Member Author

@meisenzahl meisenzahl left a comment

Choose a reason for hiding this comment

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

@danrabbit is there any way to test initial-setup in lightdm without deleting my user?


[Host name]
Identity=unix-user:lightdm
Action=org.freedesktop.hostname1.set-hostname;org.freedesktop.hostname1.set-static-hostname;
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
Action=org.freedesktop.hostname1.set-hostname;org.freedesktop.hostname1.set-static-hostname;
Action=org.freedesktop.hostname1.set-pretty-hostname;org.freedesktop.hostname1.set-static-hostname;

I'm not familiar with the pkla file format at all, unfortunately, but that might work.

Copy link
Member

Choose a reason for hiding this comment

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

The current value is fine, see here for possible values: https://www.freedesktop.org/software/systemd/man/org.freedesktop.hostname1.html#Security

@danirabbit
Copy link
Member

@meisenzahl not that I'm aware of, but as far as I can tell the userdel command doesn't delete your home directory, so if you recreate the same user it shouldn't really be noticeable. YMMV etc but that's been my experience on a test machine

Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

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

I can confirm that this works, displays the default hostname, updates the hostnames, and still creates users. 😄

@cassidyjames cassidyjames merged commit 2051efc into master Aug 20, 2021
OS 6.1 automation moved this from In progress to Done Aug 20, 2021
@cassidyjames cassidyjames deleted the set-hostname branch August 20, 2021 22:21
@cassidyjames cassidyjames mentioned this pull request Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants