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

Give the Slurm collector safer defaults #213

Merged
merged 2 commits into from
Mar 6, 2023
Merged

Conversation

rkleinem
Copy link
Collaborator

@rkleinem rkleinem commented Mar 6, 2023

As discussed. Not much to change.

If the user doesn't specify a config entry, it's good to use defaults.
If they specify something we don't know it's likely an error and we should stop.

Also, I would argue that a sensible choice for the earliest datetime is just now.
One day can be 30,000 records already...

@stefan-k
Copy link
Contributor

stefan-k commented Mar 6, 2023

Thank you for the fix!

Also, I would argue that a sensible choice for the earliest datetime is just now. One day can be 30,000 records already...

My suggestion was based on the fact that sacct itself has something similar in place when no time is specified. However, I must admit that I don't recall the specific time span. I'm fine with now, as it is the least dangerous default.

Could you fix the failing Clippy lint, please? The other CI failures are not related to your contribution.

@rkleinem
Copy link
Collaborator Author

rkleinem commented Mar 6, 2023

I removed the unused import. Is that what you meant?
Since I don't have experience with github workflows I'm not sure but it seems like clippy will always throw an error if started from a fork (actions-rs/clippy-check#2).
Or is there a way to fix that?

@stefan-k
Copy link
Contributor

stefan-k commented Mar 6, 2023

I removed the unused import. Is that what you meant?

Yes, thanks!

Since I don't have experience with github workflows I'm not sure but it seems like clippy will always throw an error if started from a fork (actions-rs/clippy-check#2). Or is there a way to fix that?

Ah sorry, yes, this is not something you should (or even can) fix. I've learned to ignore this error/warning and didn't think of mentioning it ;) Sorry!

Copy link
Contributor

@stefan-k stefan-k left a comment

Choose a reason for hiding this comment

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

Ah, now I finally understand. I thought you had a typo in the datetime string, but you had a typo in the key. Now I understand why the parsing of the datetime string didn't fail :)

LGTM.

@stefan-k stefan-k merged commit 1425321 into ALU-Schumacher:main Mar 6, 2023
@rkleinem
Copy link
Collaborator Author

rkleinem commented Mar 6, 2023

Ah, now I finally understand. I thought you had a typo in the datetime string, but you had a typo in the key. Now I understand why the parsing of the datetime string didn't fail :)

Yes, that was it. Maybe I was a little unclear.

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