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

Locale and timezone setting support #18

Merged
merged 6 commits into from
Oct 3, 2024

Conversation

slakkala
Copy link
Contributor

Description

Implement locale and timezone setting methods in givc-admin, and report the settings back to agents upon registration. Implement locale and timezone setting application in givc-agent (using localectl and timedatectl).

Checklist

  • Summary of the proposed changes in the PR description
  • More detailed description in the commit message(s)
  • Commits are squashed into relevant entities - avoid a lot of minimal dev time commits in the PR
  • Contribution guidelines followed
  • Test procedure added to nixos/tests
  • Author has run nix flake check --accept-flake-config and it passes
  • All automatic Github Action checks pass - see actions
  • Author has added reviewers and removed PR draft status

Testing

@slakkala slakkala requested review from mbssrc and avnik September 25, 2024 11:04
Add SetLocale and SetTimezone methods to givc-admin for changing the system settings.
When set, the value are stored to a configuration file, and reported back to agents
when they register.

Adjust givc-agent to handle the timezone and locale settings repotrted by the admin; use
systemd's localectl, timedatectl and systemctl set-environment to apply the settings. Add
policykit policy to allow locale and timezone setting to the givc user.

Signed-off-by: Santtu Lakkala <[email protected]>
Change locale and timezone to be signalled from admin to agent after registration.

Signed-off-by: Santtu Lakkala <[email protected]>
Signed-off-by: Santtu Lakkala <[email protected]>
@slakkala slakkala force-pushed the dev/locale-timezone branch from 0ff57cd to c7894e9 Compare October 1, 2024 10:38
Copy link
Collaborator

@mbssrc mbssrc left a comment

Choose a reason for hiding this comment

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

Let's add a test case for this later

Copy link
Contributor

@avnik avnik left a comment

Choose a reason for hiding this comment

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

Generally LGTM, I planning to approve it as soon as you resolve some small questions.

Also next time please split feature PR and massive style refactoring PR (I do this mistake often myself as well)

use tokio_stream::StreamExt;
use tonic::transport::Channel;
use tracing::debug;

use givc_common::address::EndpointAddress;
use givc_common::pb;
Copy link
Contributor

Choose a reason for hiding this comment

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

I (started) grouped imports intentionally -- std, libraries, project crates, this crates import (internally alphabetically sorted)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, this got messed up in a rebase.

enable = true;
extraConfig = ''
polkit.addRule(function(action, subject) {
if (action.id == "org.freedesktop.locale1.set-locale" && subject.user == "ghaf") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should username be hardcoded? Or we should have ghaf.username setting somewhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question; I'll check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That broke standalone things, reverting for now.

task: tokio::task::JoinHandle<()>,
}

impl Drop for WatchResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am concerning about remove Drop here, I not sure if task spawned for polling grpc would shut down automatically, if WatchResult is destroyed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the Drop implementation, the spawned task will run until there is an event, at which point the recv will fail and task will terminate.

This was mainly for larger refactoring for helping interoperability with gtk-rs, but turned out it actually made things worse.

My main concern with this is that the WatchResult cannot be destructured, but I guess we can live with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it now to use a tokio mpsc channel for signalling; no need to explicitly implement Drop: when the (last, but in this case only) sender is dropped, the recv() -future will complete, and the task will terminate.

@slakkala
Copy link
Contributor Author

slakkala commented Oct 2, 2024

Also next time please split feature PR and massive style refactoring PR (I do this mistake often myself as well)

I did intentionally put them in separate commits, but yeah, maybe PRs would've been smarter.

Signed-off-by: Santtu Lakkala <[email protected]>
@slakkala slakkala force-pushed the dev/locale-timezone branch 2 times, most recently from 5460358 to e2ceb07 Compare October 3, 2024 07:47
@mbssrc mbssrc merged commit 68079de into tiiuae:main Oct 3, 2024
4 checks passed
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.

3 participants