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.

Sign up for GitHub

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

Jump to bottom

Slack alert throttle #15

Merged
merged 14 commits into from
Nov 18, 2024
Merged

Slack alert throttle #15

merged 14 commits into from
Nov 18, 2024

Conversation

neoformit
Copy link
Collaborator

@neoformit neoformit commented Nov 12, 2024 •
edited
Loading

Resolves #14

  • Throttle slack alerts, defaults to 1 per week per JWD
  • Keep a history of slack notifications in temp file (defaults to /tmp/walle-notifications.txt)

Not sure if these should live in the Ansible repo? I could just gitignore them?

  • Add some tests
  • Add requirements.txt

@mira-miracoli
Copy link
Contributor

Not sure if these should live in the Ansible repo? I could just gitignore them?

Sorry I don't really see why, maybe you can explain more about that? (or maybe it is obvious but I did not see it) also we need to make sure there is no user data involved.
Is it not possible that this lives on the node?

@neoformit
Copy link
Collaborator Author

maybe you can explain more about that?

It's only that the test.py and requirements.txt files are for development and not something that needs to be ansible-galaxy installed by everyone. I don't mind if they live in here if you're happy!

make sure there is no user data involved.

If you mean the NotificationHistory class, it only stores date strings and JWD path strings in the history file.

@mira-miracoli
Copy link
Contributor

ah okay, sorry I misunderstood and thought that you wanted to store the slack history in the github repo.
yes the requirements and tests of course can live here and also it is nice to have the requirements if people use the virtualenv method! :)

@mira-miracoli
Copy link
Contributor

Sorry I have another question:
Was there a specific reason you used the JWD for the notification history and not the galaxy_id (job id)?

mira-miracoli
mira-miracoli reviewed Nov 13, 2024
View reviewed changes
files/test.py Show resolved Hide resolved
files/requirements.txt Outdated Show resolved Hide resolved
files/walle.py Outdated Show resolved Hide resolved
@neoformit
Copy link
Collaborator Author

Sorry I have another question: Was there a specific reason you used the JWD for the notification history and not the galaxy_id (job id)?

Nope, it was just the most obvious identifier to me at the time. Job ID would be fine too!

@neoformit
Copy link
Collaborator Author

@mira-miracoli happy for me to merge this?

mira-miracoli
mira-miracoli approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@mira-miracoli mira-miracoli left a comment

Choose a reason for hiding this comment

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

Yes! Sorry for being annoying and nit-picky

files/walle.py
@@ -63,6 +64,20 @@ def convert_arg_to_seconds(hours: str) -> float:
return float(hours) * 60 * 60


@dataclass
class Record:
Copy link
Contributor

Choose a reason for hiding this comment

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

cool, thanks a lot 😊

@mira-miracoli mira-miracoli merged commit 4cbd41a into usegalaxy-eu:main Nov 18, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mira-miracoli mira-miracoli mira-miracoli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Slack alerts option multiple alerts
2 participants
@neoformit @mira-miracoli