Skip to content

NodeSet: add special notation @@source to expand group names #468

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

Merged
merged 1 commit into from
Jun 19, 2022

Conversation

thiell
Copy link
Collaborator

@thiell thiell commented Feb 8, 2022

This change allows access to all group names of a specific source in
node set expressions. The special operator @@ followed by the source
expands group names that can then be manipulated as a set.

Note: In that case, group names are not prefixed with @ and as such, not
further expanded into nodes by NodeSet.

@degremont
Copy link
Collaborator

Interesting feature! I'm trying to have this match into the other feature we have for NodeSet syntax/arithmetic.

So far, we supports various syntaxes to identify node name, or group names that will be mapped into node names.
Group name related syntax refers to the current default source by default. An additional prefix could be added to point to a different source.

I like the idea of fetching the group list with a nodeset syntax, but I would like it to have a syntax that could be pointing to the current source without knowing its name.

  • Syntax to get the group name of current source
  • Syntax to get the group name for source B. Ideally this syntax is the same than the other, where we just add the source name prefix source:.

@degremont degremont self-requested a review March 3, 2022 13:20
@thiell
Copy link
Collaborator Author

thiell commented Mar 4, 2022

Thanks! This patch implements your second point. Note that when listing group names from a source, only the source name is needed. For example, with this patch, @@B will get you the group names for source B.

With the @source:group syntax, the input parameter is mainly the group name. With @@ however, it's the source itself. I can propose to see if we can implement@@ (not followed by an explicit source name) what would resolve group names of the default/current source.

Lastly, but I doubt it will be very useful in practice, we could implement @@source:<group_pattern>, to filter the group names based on the wildcard pattern group_pattern, like @@B:pdu*.

Both last ideas are compatible with this patch. Let me know what you think.

@kcgthb
Copy link
Contributor

kcgthb commented Mar 4, 2022

I like it the way it is right now, not only because the patch is just so tiny and elegant (which talks to the quality of the existing code), but also because I feel it's more intuitive for users.

Listing the group names in group source foo with cluset -e @@foo seems pretty easy to grasp.

I'm not sure implementing a @@source:<group_pattern> filter is worth the trouble, it's a job better left of for grep and friends, IMHO.

And as for listing the groups in the current source without knowing its name, it works out the box already with:

cluset -e @@

Did I mention that patch was awesome? ;)

Copy link
Collaborator

@degremont degremont left a comment

Choose a reason for hiding this comment

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

Ok, if you both think @@ is a clear enough syntax, i'm not against it. Just make that clear in the doc and the tests that this is an officially supported syntax.

Agreed that name filtering could be added later, if people asks for it.

@@ -916,6 +916,8 @@ def parse_group_string(self, nodegroup, namespace=None):
namespace, group = grpstr.split(':', 1)
if group == '*': # @* or @source:* magic
reslist = self.all_nodes(namespace)
elif group.startswith('@'): # @@source group name list
reslist = self.grouplist(grpstr[1:])
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, no source name is supported because grpstr[1:] will be expanded to '' and we are testing namespace as a boolean, so both None (official value) and '' will evaluate to False.

@thiell thiell force-pushed the b185_nsgroupiter branch from 12d2d00 to 715cdb3 Compare March 19, 2022 23:58
@thiell thiell self-assigned this Mar 20, 2022
@thiell thiell added this to the 1.8.5 milestone Mar 20, 2022
This change allows access to all group names of a specific source in
node set expressions. The special operator `@@` optionally followed
by a source expands group names that can then be manipulated as a set.
The default group source is used when `@@` is used alone.

Note: In that case, group names are not prefixed with @ and as such, not
further expanded into nodes by NodeSet.
@thiell thiell force-pushed the b185_nsgroupiter branch from 715cdb3 to e995f4c Compare March 20, 2022 00:24
@thiell
Copy link
Collaborator Author

thiell commented Mar 20, 2022

Just make that clear in the doc and the tests that this is an officially supported syntax.

done!

@thiell thiell merged commit 5d63007 into cea-hpc:master Jun 19, 2022
@thiell thiell modified the milestones: 1.8.5, 1.9 Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants