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

Create bors.cleanup task #359

Merged
merged 1 commit into from Mar 6, 2018

Conversation

Projects
None yet
3 participants
@grahamc
Contributor

grahamc commented Mar 5, 2018

I don't quite follow the meaning of:

make it accessible like migrations are, so that the people using it in Docker and the people doing it in Heroku can both run it

but I was able to run this on heroku okay.

Implements #313

@notriddle

This comment has been minimized.

Member

notriddle commented Mar 5, 2018

CC @danielkza because Daniel made the Ecto migrations available to people who couldn't run Mix tasks, and the cleanup task should also be accessible to such users.

@danielkza

This comment has been minimized.

Contributor

danielkza commented Mar 6, 2018

@grahamc The production Docker image is created by building an OTP release with Distillery, which does not include Mix itself, only the compiled Bors code. I don't think Heroku does the same, but just runs directly from the source instead.

In your current version, the task would not be accessible in the compiled release. Unfortunately I'm far from an Elixir expert, and couldn't find an easy way to share code from our project (Bors) and include it in a Mix task. :(

@notriddle

This comment has been minimized.

Member

notriddle commented Mar 6, 2018

If that's true, then I guess we can just merge this as-is.

One more thing; could you rebase this onto master? It's still putting the file into apps/bors_database, which doesn't exist any more.

@grahamc grahamc force-pushed the tweag:cleanup-task branch from f27d9b4 to a0998af Mar 6, 2018

@grahamc

This comment has been minimized.

Contributor

grahamc commented Mar 6, 2018

Gotcha. Hmm... I wonder if it would be better to use however the dev version is built, so it does include mix? Surely bors doesn't strictly require whatever performance benefits are had from a "production" build.

I rebased, @notriddle. Thanks!

@grahamc

This comment has been minimized.

Contributor

grahamc commented Mar 6, 2018

Prior to merging, I'd like to test this on one more deployment.

@danielkza

This comment has been minimized.

Contributor

danielkza commented Mar 6, 2018

@grahamc The point of using a compiled release with multi-stage Dockerfile is to reduce the size of the container image. IIRC including all the Elixir and Node dependencies adds hundreds of MBs to the final size.

@grahamc

This comment has been minimized.

Contributor

grahamc commented Mar 6, 2018

[nix-shell]$ psql bors_dev
psql (9.6.8)
Type "help" for help.

bors_dev=# select count(1) from patches;
 count 
-------
   135
(1 row)

bors_dev=# select count(1) from batches;
 count 
-------
   101
(1 row)

[nix-shell]$ MIX_ENV=dev mix bors.cleanup  --months 1
Compiling 1 file (.ex)
Cleaning data older than 1 months
[debug] QUERY OK source="batches" db=13.5ms queue=16.8ms
DELETE FROM "batches" AS b0 WHERE (b0."state" IN ($1,$2,$3) AND (b0."updated_at" < ($4::timestamp + ($5::numeric * interval '1 month'))::timestamp)) [2, 3, 4, {{2018, 3, 6}, {18, 27, 43, 482559}}, #Decimal<-1>]
[debug] QUERY OK source="patches" db=11.4ms
DELETE FROM "patches" AS p0 WHERE (NOT (p0."open") AND (p0."updated_at" < ($1::timestamp + ($2::numeric * interval '1 month'))::timestamp)) [{{2018, 3, 6}, {18, 27, 43, 560249}}, #Decimal<-1>]

[nix-shell]$ psql bors_dev
psql (9.6.8)
Type "help" for help.

bors_dev=# select count(1) from patches;
 count 
-------
    78
(1 row)

bors_dev=# select count(1) from batches;
 count 
-------
    43
(1 row)

LGTM

@notriddle

This comment has been minimized.

Member

notriddle commented Mar 6, 2018

bors r+

bors bot added a commit that referenced this pull request Mar 6, 2018

Merge #359
359: Create bors.cleanup task r=notriddle a=grahamc

I don't quite follow the meaning of:

> make it accessible like migrations are, so that the people using it in Docker and the people doing it in Heroku can both run it

but I was able to run this on heroku okay.

Implements #313
@bors

This comment has been minimized.

Contributor

bors bot commented Mar 6, 2018

@bors bors bot merged commit a0998af into bors-ng:master Mar 6, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
bors Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@grahamc grahamc deleted the tweag:cleanup-task branch Mar 7, 2018

notriddle added a commit to bors-ng/bors-ng.github.io that referenced this pull request Mar 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment