-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Alexgithublab: change time (superseeds #1730) #1737
Conversation
@JonathonHall-Purism please review |
This comment was marked as off-topic.
This comment was marked as off-topic.
Signed-off-by: Thierry Laurion <[email protected]>
Signed-off-by: Thierry Laurion <[email protected]>
Signed-off-by: Thierry Laurion <[email protected]>
Signed-off-by: Thierry Laurion <[email protected]> Signed-off-by: Thierry Laurion <[email protected]>
@alexgithublab your commits were not signed (DCO failing on github action see your ofirinal PR), force pushing to limit maintainership time and back and forths, please review https://github.com/linuxboot/heads/blob/master/CONTRIBUTING.md and the changes in this PR. |
0b0f2e8
to
2189636
Compare
…ove unneeded duplicate menu options to change system time Signed-off-by: Thierry Laurion <[email protected]>
2189636
to
f4ce047
Compare
A gentle reminder that you can do those tests and generate the same screenshots i'm doing here by iteratively testing UX changes and see if they fit smaller screens and all by simply doing (with qemu variants of your choice). It would have cought your chmox x missing on change-time and also shows that you weren't aware of the TOTP mismatch already existing path. Since system time is mostly used in time CMOS mattery is dead/when laptop wasn't boot since a long time and or without network access so that main OS syncs time through NTP automatically, there might be some doc in code/prompts missing still. The current UX takes for granted users know how TOTP works and NTP works. Might be an error doing so and create more issues opened while we could prevent this working a little more on it now then later. Your choice since you are a support provider through Nitrokey support channels and this affects older hardware more than newer ones, which Nitrokey is still a reseller. You can iteratively test your changes with:
New option under Options menu (New UX): @alexgithublab this is nice improvement in UX, thanks for your contribution. |
Sorry for the DCO.. It's weird bc I see my commits signed, I don't know what I'm missing here. Thanks for the reminder.
Not necessary for example on new ones Ethernet cable is not working within Heads, now we have USB tethering but this not very convenient to set and also you need to have a compatible phone. |
DCO error goes in details on why its being triggers, clicking on it should answer most of your questions better then I could attempt without pertinence. TLDR: this is git
Sorry, I meant clock drifting is more related to older hardware, at least form my experience going over and over explaining this, both because drifting seems to go faster on old platforms and also that older platforms seem to have a tendency to be less often connected to network and as a result less NTP time corrected through wifi. Yes, Ethernet is fading out, as for Tethering, please open issues to facilitate its usage or comment existing issue to improve its documentation at linuxboot/heads-wiki#149 or open a code/pr to improve UX directly in code. @alexgithublab are you ok with the changes in this PR against your original code?
Is short, is this ready to be reviewed by other stakeholders to your eyes because Heads master shared between many OEM now, so this needs to be ok between us and reviewed with other suggestions taken so that UX is simplified, not complexified more and none of us are still UX experts. Would be nice if one was appointed to help us improve things in this area, I know none. |
@alexgithublab if you're ok with my changes, please approve this PR to that other stakeholders can approve. Also please test this into qemu so that you can aksowledge what is expected by contributors for testing their changes themselves for future PR from your side and reduce my involvement into finding chmod +x issues, then functional testing, then exposing UX changes then doing code review etc. |
@alexgithublab maybe change-time should remind actual system clock prior of change? |
@JonathonHall-Purism ping |
Sorry for the delay here but I'm taking a look, nice work all. I would like to propose some wording improvements, will test and provide a commit, should be ready tomorrow. |
@tlaurion @alexgithublab I made some improvements and proposed them in #1748. Thanks for all the work on this! If you'd rather pull changes over to this PR though (and/or discuss of course), that's fine too. |
…ments Alexgithublab: change time, 3.0 (supersedes #1737)
This is PR superseding #1730
This add helper to change system time clock engaging user into coonsulting external source of time in UTC and setting system time accordingly quite easily, thanks to @alex-nitrokey contribution under #1730.
Screenshots of changed UX at #1737 (comment)
@alex-nitrokey please comment and approve PR.
@JonathonHall-Purism please acknowledge/suggest changes so that you are ok with wording prior of merge.