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

Prune bulk email verification database #1377

Merged
merged 18 commits into from
Dec 4, 2023
Merged

Conversation

par4m
Copy link
Contributor

@par4m par4m commented Nov 14, 2023

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

LGTM overall! Only small nits

script/Cargo.lock Outdated Show resolved Hide resolved
script/src/main.rs Outdated Show resolved Hide resolved
script/Cargo.toml Outdated Show resolved Hide resolved
@par4m
Copy link
Contributor Author

par4m commented Nov 15, 2023

Would you be interested in automated tests ? Running the script as a service instead of a one-time cleanup

@amaury1093
Copy link
Member

Would you be interested in automated tests ?

You mean a rust #[test]? I don't think it's necessary for this function.

Running the script as a service instead of a one-time cleanup

I am also thinking about this. Currently the way I see the end user using it is to create a binary (via cargo build), and put it in a crontab. Or maybe a docker container.

What do you mean by "run as a service"?

@amaury1093
Copy link
Member

Can you also fix the failing tests here?

@amaury1093
Copy link
Member

Not sure if the failing CI action is due to code change or a simple fluke. Could you resolve conflicts and let's retry?

@par4m
Copy link
Contributor Author

par4m commented Nov 16, 2023

Not sure if the failing CI action is due to code change or a simple fluke. Could you resolve conflicts and let's retry?

Seems like a fluke, I've barely changed anything and as far as I know the tests had passed on the first commit ? I'll check the logs and try to fix it

@par4m
Copy link
Contributor Author

par4m commented Nov 18, 2023

I've resolved the conflicts, let's see if the tests pass now.

@par4m
Copy link
Contributor Author

par4m commented Nov 19, 2023

Seems like an openssl-sys issue

sfackler/rust-openssl#1021 (comment)
sfackler/rust-openssl#1021 (comment)

@amaury1093
Copy link
Member

Do you have any ideas how to fix it in the Github action file?

What worries me is that this same action passes on Github, and on other PRs like #1370, so it is related to this PR?

@par4m
Copy link
Contributor Author

par4m commented Nov 21, 2023

Do you have any ideas how to fix it in the Github action file?

What worries me is that this same action passes on Github, and on other PRs like #1370, so it is related to this PR?

Seems like a very prominent issue https://github.com/sfackler/rust-openssl/issues?q=is%3Aissue++action+

script/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@twitu twitu left a comment

Choose a reason for hiding this comment

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

@par4m Consider merging he pruning logic into the backend crate. This has a couple of advantages -

  • One less crate and dependencies list to maintain. Most of the dependencies are the same as the ones used for bulk logic in backend. This will potentially solve the ssl issue as well since backend already has an ssl dependency that passes.
  • This will also make pruning flexible
    • It can be feature flagged and launched as part of the main script in backend using tokio::spawn.
    • It can also be exposed as a cli command by creating a src/bin/prune_db.rs in backend. This will allow users to do one off pruning or build the executable and run it as a cron job.

@amaury1093
Copy link
Member

Weird that the Action is still failing. I'll merge this PR, I'll figure out later.

Two things before I merge (same comments as #1377 (review), seems like they were reverted):

  • can you rename DEBUG_MODE to DRY_RUN
  • can you add a small paragraph in the backend's README?

@amaury1093
Copy link
Member

Thanks! Merging, let's hope the failing check was just a fluke on this PR.

@amaury1093 amaury1093 enabled auto-merge (squash) December 4, 2023 14:28
@amaury1093 amaury1093 disabled auto-merge December 4, 2023 14:28
@amaury1093 amaury1093 merged commit f905735 into reacherhq:master Dec 4, 2023
9 of 13 checks passed
This was referenced Dec 4, 2023
juhniorsantos pushed a commit to juhniorsantos/check-if-email-exists that referenced this pull request Apr 11, 2024
* Add cleanup script: Delete completed jobs

* Add Env var. Switch to SQL only

* Add Transactions. println -> info!

* Fix workspace member and reformat

* Add debug mode

* Minor fixes

* rename to dry_mode

* Delete script/Cargo.lock

* Add script/README

* Fix

* update Cargo.lock

* update script/Cargo.toml

* update dependency

* update dependency

* move script -> backend

* update backend/README

---------

Co-authored-by: Ishan Bhanuka <bhanuka.ishan@gmail.com>
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.

4 participants