-
Notifications
You must be signed in to change notification settings - Fork 3
Fix/Improve class With #2
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
base: main
Are you sure you want to change the base?
Conversation
thanks for the great ansible slurm.py plugin! |
Do you have an example use case for when this is useful? I think this could be simplified a bit, and maybe things have changed slightly, but sacctmgr doesn't let you specify, say, |
Some simplified versions: - name: "Ensure that user exists"
slurm:
entity: user
state: present
name: "root"
args:
defaultaccount: "foo"
account: "test"
cluster: "bar"
run_once: true # noqa run-once[task] This not any more - name: "Ensure that user exists"
slurm:
entity: user
state: present
name: "root"
args:
defaultaccount: "foo"
cluster: "bar"
run_once: true # noqa run-once[task] in modified version both work. or do I miss something here? |
Ah, hm, it seems like sacctmgr will infer the account from defaultaccount. If you don't specify defaultaccount, this gives an error |
And the similar statement holds for the rest |
By the way it also works when |
That is the idea. These were originally derived from the sacctmgr argument parsing source, but from a very old slurm version so some things may be out of date. But in general, yes, sacctmgr only allows you to specify association parameters when you specify a specific association. There are cases where it will infer or just apply to all associations, but you probably don't want you configuration management system changing things without being specific about what they are. |
I think that, fixing the second element name explicitly does not match any more to the current sacctmgr behavior. One should also consider what the ansible check mode reports. Honestly, I am not pretending that my solution is better, but currently either I should restrict everything explicitly or accept that default values are also there. |
Current behavior:
One should explicitly provide the second element of With class in order to be able to use any of following ones. Kind of the name of the second element is hard coded.
Improved behavior:
Does not matter any more which of the following elements are provided.