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

loginctl: list-{users,sessions}: add a column for showing state #27606

Merged
merged 4 commits into from
May 16, 2023

Conversation

YHNdnzj
Copy link
Member

@YHNdnzj YHNdnzj commented May 11, 2023

No description provided.

@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label May 11, 2023
@YHNdnzj YHNdnzj added the login label May 11, 2023
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

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

LGTM.

Could you add a brief test for the fields in TEST-73-LOCALE ?

@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels May 15, 2023
@github-actions github-actions bot added tests please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 15, 2023
@dtardon
Copy link
Collaborator

dtardon commented May 15, 2023

@YHNdnzj I wonder: is the state really so useful to users that it needs to be listed? It seems nobody has ever asked for this...

@YHNdnzj
Copy link
Member Author

YHNdnzj commented May 15, 2023

@YHNdnzj I wonder: is the state really so useful to users that it needs to be listed? It seems nobody has ever asked for this...

I personally encountered people asking for this :)

Also, before #26744 is solved, at times the output could become pretty useless if the state is not shown.

@yuwata yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels May 15, 2023
@bluca bluca added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels May 15, 2023
@YHNdnzj
Copy link
Member Author

YHNdnzj commented May 16, 2023

@bluca I don't think the CI failures are related? Lint Code Base reports on code not changed in this PR, and jammy-ppc64el fails due to network errors.

@mrc0mmand
Copy link
Member

@bluca I don't think the CI failures are related? Lint Code Base reports on code not changed in this PR, and jammy-ppc64el fails due to network errors.

Don't worry about the shellcheck stuff - a new version of shellcheck got out with additional checks. I'll fix it when I do another round of shell-related fixes.

@YHNdnzj YHNdnzj added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels May 16, 2023
src/login/loginctl.c Outdated Show resolved Hide resolved
src/login/loginctl.c Outdated Show resolved Hide resolved
@poettering poettering added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels May 16, 2023
@YHNdnzj YHNdnzj force-pushed the loginctl-list-show-state branch 3 times, most recently from 23d2757 to 82a7c53 Compare May 16, 2023 10:07
@poettering
Copy link
Member

one of those days, if we keep adding properties to the table we should add a new version of the ListSession() and other methods that just pass the data along entirely. The many roundtrips really suck.

but that is for another time i guess.

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels May 16, 2023
@poettering
Copy link
Member

lgtm

@yuwata yuwata merged commit 871a41f into systemd:main May 16, 2023
42 of 47 checks passed
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label May 16, 2023
@YHNdnzj YHNdnzj deleted the loginctl-list-show-state branch May 16, 2023 16:43
dustymabe added a commit to dustymabe/zincati that referenced this pull request Aug 15, 2023
In systemd 254 the `loginctl list-sessions --json` output was modified slightly:

Before
```
[{"session":"4","uid":1000,"user":"core","seat":"","tty":""}]
```

After
```
[{"session":"17","uid":1000,"user":"core","seat":"","tty":"n/a","state":"active","idle":false,"since":null}]
```

Notice the change from `""` to `"n/a"` for the `tty` field.

This change probably happened in one of:

- systemd/systemd#27769
- systemd/systemd#27740
- systemd/systemd#27606

Let's update the code to handle the new case.

Fixes coreos/fedora-coreos-tracker#1547
dustymabe added a commit to dustymabe/zincati that referenced this pull request Aug 15, 2023
In systemd 254 the `loginctl list-sessions --json` output was modified slightly:

Before
```
[{"session":"4","uid":1000,"user":"core","seat":"","tty":""}]
```

After
```
[{"session":"17","uid":1000,"user":"core","seat":"","tty":"n/a","state":"active","idle":false,"since":null}]
```

Notice the change from `""` to `"n/a"` for the `tty` field.

This change probably happened in one of:

- systemd/systemd#27769
- systemd/systemd#27740
- systemd/systemd#27606

Let's update the code to handle the new case and rename the function to
be a little more appropriate since the use case for it is narrow.

Fixes coreos/fedora-coreos-tracker#1547
dustymabe added a commit to dustymabe/zincati that referenced this pull request Aug 15, 2023
In systemd 254 the `loginctl list-sessions --json` output was modified slightly:

Before
```
[{"session":"4","uid":1000,"user":"core","seat":"","tty":""}]
```

After
```
[{"session":"17","uid":1000,"user":"core","seat":"","tty":"n/a","state":"active","idle":false,"since":null}]
```

Notice the change from `""` to `"n/a"` for the `tty` field.

This change probably happened in one of:

- systemd/systemd#27769
- systemd/systemd#27740
- systemd/systemd#27606

Let's update the code to handle the new case and rename the function to
be a little more appropriate since the use case for it is narrow.

Fixes coreos/fedora-coreos-tracker#1547
dustymabe added a commit to dustymabe/zincati that referenced this pull request Aug 15, 2023
In systemd 254 the `loginctl list-sessions --json` output was modified slightly:

Before
```
[{"session":"4","uid":1000,"user":"core","seat":"","tty":""}]
```

After
```
[{"session":"17","uid":1000,"user":"core","seat":"","tty":"n/a","state":"active","idle":false,"since":null}]
```

Notice the change from `""` to `"n/a"` for the `tty` field.

This change probably happened in one of:

- systemd/systemd#27769
- systemd/systemd#27740
- systemd/systemd#27606

Let's update the code to handle the new case and rename the function to
be a little more appropriate since the use case for it is narrow.

Fixes coreos/fedora-coreos-tracker#1547
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants