-
Notifications
You must be signed in to change notification settings - Fork 136
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
Remove nix #291
base: master
Are you sure you want to change the base?
Remove nix #291
Conversation
5018477
to
8dab6c7
Compare
I'm not a maintainer, but I suspect this would be more likely to be accepted if it adds a plain Dockerfile rather without removing the Nix setup. This could include removing the dockerize stuff from the Nix setup, but not removing the Nix setup altogether. As was discussed on #288, there are still users who prefer to deploy marge-bot via Nix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also say points 2 and 5 could be split into their own PR that gets merged first, to make the purpose of this PR clearer (basically cherry-pick fdd7721 but without the dependency upgrades).
And just my opinion but at this point GitHub Actions are probably nicer for quick CI tests than Travis ;) At least in my experience as of late 2020 Travis quality and job queues have deteriorated. Maybe not for deployment yet, as you have secrets stored already from what I see.
@nejch thanks for the feedback. I've submitted a new MR to upgrade the dependencies and fix lint warnings introduced by the latest linters. However other parts, e.g. testing with more python versions, plain-Dockerfile, are all heavily based on the removal of nix. I'm not sure if it's viable to rewrite them along with the nix stuff. |
Yeah that's basically what I had in mind for splitting up the linting issues, thanks :) |
9c10ea8
to
0452c4c
Compare
Resolves #288