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

Feat : alerts module #95

Merged
merged 21 commits into from
Aug 30, 2024
Merged

Feat : alerts module #95

merged 21 commits into from
Aug 30, 2024

Conversation

ocdbytes
Copy link
Member

  • Added alerts module.
  • Added SNS implementation.
  • Updated tests.

@ocdbytes ocdbytes changed the title Feat : alerts module [In Progress] Feat : alerts module Aug 22, 2024
@ocdbytes ocdbytes added the enhancement New feature or request label Aug 22, 2024
@coveralls
Copy link

coveralls commented Aug 22, 2024

Coverage Status

coverage: 64.606% (-0.4%) from 64.983%
when pulling 8d41f39 on feat/alerts-module
into 77bf9c6 on main.

@apoorvsadana apoorvsadana changed the title [In Progress] Feat : alerts module Feat : alerts module Aug 22, 2024
Comment on lines +15 to +16
let config = aws_config::from_env().region(Region::new(sns_region)).load().await;
AWSSNS { client: Client::new(&config) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I was comparing this to the S3 config and I noticed a few things

  1. We can use the same config built here in S3 as well (we will need to change Client::from_conf to Client::new inside s3)
  2. We should probably move AWS config creation into config.rs and pass it to s3 and sns
  3. s3 new doesn't need to async (and even SNS once can be made non sync after that)
  4. S3 config only needs to have bucket name after this
  5. SNS should also have a config that has topic arn which then gets loaded into the SNS struct (similar to S3)

Copy link
Member Author

Choose a reason for hiding this comment

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

we can make a new issue for this idts this is related to this PR and will cause unnecessary confusion amongst the reviewers.

Copy link
Contributor

Choose a reason for hiding this comment

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

resolved in #98

crates/orchestrator/src/alerts/aws_sns/mod.rs Show resolved Hide resolved
crates/orchestrator/src/jobs/mod.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/queue/job_queue.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/tests/alerts/mod.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/tests/common/mod.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/tests/jobs/mod.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/tests/jobs/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@antiyro antiyro left a comment

Choose a reason for hiding this comment

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

lgtm

@ocdbytes ocdbytes merged commit f84ba41 into main Aug 30, 2024
8 checks passed
@ocdbytes ocdbytes deleted the feat/alerts-module branch August 30, 2024 06:48
ocdbytes added a commit that referenced this pull request Oct 16, 2024
* feat : alerts module init

* feat : added alerts in jobs/mod.rs

* feat : updated messages

* feat : updated tests with alerts module mock

* changelog : update

* feat : added additional alerts

* feat : added test for alerts and added logs at queue level

* refactor : removed redundant alerts

* feat : updated workflow

* feat : resolved comments

* feat : added all alerts for queues

* feat : updated code

* remove queue url const

* feat : tests and lint fixes

---------

Co-authored-by: apoorvsadana <95699312+apoorvsadana@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants