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

remote/client: consider acquired places on search #1608

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gumulka
Copy link

@gumulka gumulka commented Feb 13, 2025

When requesting an acquired place, match the pattern only to the acquired places and not to every place.
This also allows to skip the place on the command line if only one place is acquired by the current user.

Description

Checklist

  • Documentation for the feature
  • Tests for the feature
  • PR has been tested

I skipped documentation and automatic tests for this feature and I'm not sure about the documentation, because it is a change on how the client behaves from a user perspective.

On the one hand, the behavior before was not documented and only know through usage, the behavior afterwards will also only be known through usage and is not documented. Not sure on where to document it either.

Test case locally was that I created three boards with an serial port:

  • board-1
  • board-2
  • other-1
    Afterwards, I acquired board-1 and other-1, then called labgrid-client console, which resulted in an error, that there are two places acquired.
    labgrid-client -p board console resulted in the console and after releasing board-1, then labgrid-client console resulted in a console.

@Emantor
Copy link
Member

Emantor commented Feb 17, 2025

This change is missing a DCO. Additionally this change uses "" to retrieve all places which currently works since the remote client synchronizes all place information from the coordinator. The longterm plan for the client is to move the blanket * subscription of all place information to a more specific subscription to places that match the pattern specified by the client.

This change would make this harder to implement, since to enumerate all possible acquired places for a user we have to request all places or change the subscription mechanism.

Note that the place pattern can not only be specified with the -p command line option, but also via the LG_PLACE environment variable. This makes client usage on the CLI more straightforward.

When requesting an acquired place, match the pattern only to the
acquired places and not to every place.
This also allows to skip the place on the commandline if only one
place is acquired by the current user.

Signed-off-by: Fabian Pflug <[email protected]>
@gumulka gumulka force-pushed the pattern_acquired_places branch from b49d93f to 92d831a Compare February 18, 2025 09:49
@gumulka
Copy link
Author

gumulka commented Feb 18, 2025

"" is used, because it matches every pattern and is what is working with the current codebase without rewriting more parts about getting places. I could not look into the future of the project when writing it.

I'm well aware of the environment variable. but since I mostly only have one place acquired, but I'm working with multiple terminals, it is easier to not think about what I have acquired and just accept it.
Currently, I have export LG_PLACE=$(labgrid-client who | grep $(hostname) | grep $USER | xargs | cut -d " " -f 3) in my history to accomplish what this PR does for me with one command more, but it would have been nice to get rid of one command. (also this fails, if there are two or more place acquired and returns the first place, this safetycheck is nice in the PR for me)

If it is in the way for future development and is hindering it, I'm okay with not accepting it.

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.

2 participants