alces-software / adminware

A sandbox CLI for running commands remotely across nodes
1 stars 0 forks source link

Feat/stack node arguments #73

Closed DavidMarchant closed 5 years ago

DavidMarchant commented 5 years ago

Closes #68 when merged Now can have >1 nodes and >1 groups specified (in any order!)

Any duplicate nodes included after these are evaluated are removed so each is only executed on once (aliases notwithstanding) Also node ranges are supported!

WilliamMcCumstie commented 5 years ago

Rebased on develop to resolve the conflict with the click CLI. It was created by a change in the comment on the develop branch

WilliamMcCumstie commented 5 years ago

I don't think using re to preform the expansion is a good idea. Currently genders is used to expand the groups into nodes. It also handles expanding node[01-10] to node01, node02 etc...

By re-implementing the expansion with re, it opens up the possibility it will not match genders behaviour.

Instead the expansion should be completely done with either re or genders. If they genders approach is taken, a temporary file can be created with the nodes (sans groups). Then the nodeattr -f <file> --expand option can be used to expand it. No groups are required for this.

Alternatively, all the expansions can be done using re, but this would require reworking how groups are specified

DavidMarchant commented 5 years ago

Ah right - I had read that genders files handle ranges themselves but it completely left my brain as soon as I read it. I see no reason not to implement this using nodeattr, the possibility of mismatch is very real, I'll get on with looking about how to do this now. Cheers! (but all my pretty regex :sob:)

mjtko commented 5 years ago

We don't want a hard dependency on nodeattr/genders so avoid that if it's looking like it is, rather than through an abstract API or something.

DavidMarchant commented 5 years ago

Ok Mark, i think building a file & then parsing it back down as a genders file would constitute a hard dependancy, what course of action would you recommend? I'm not sure that there is an abstract API for genders, I could attempt to recreate the genders policy as best I can using regex? We have hard dependency on genders elsewhere though (for all group related stuff), is this an issue too?

mjtko commented 5 years ago

IIRC we'd agreed that any "genders specific" stuff was to be abstracted in a switch-out-able way so we weren't tying ourselves into genders. @WilliamMcCumstie: do you remember that discussion?

WilliamMcCumstie commented 5 years ago

We already have a hard dependency on nodeattr in order to make the groups work. Currently there is a groups module that abstracts nodesattr behaviour away. It would be a simple case of adding a expand_node_group(node_notation) that preforms the temporary file creation and nodeattr expansion.

This way all the group/node expansion code is done in a single location. Then if we want to remove nodeattr in the future, then only the groups module will need revamping.

DavidMarchant commented 5 years ago

oh sure thing! on it

WilliamMcCumstie commented 5 years ago

Rebased to remove the merge commit as it is duplicating the commits on this branch

WilliamMcCumstie commented 5 years ago

Rebased on develop to remove the merge conflict

DavidMarchant commented 5 years ago

Some changes made - however haven't made a nodes alias for node yet