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

Remove nix #291

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove nix #291

wants to merge 2 commits into from

Conversation

qqshfox
Copy link
Contributor

@qqshfox qqshfox commented Feb 23, 2021

  1. Introduce Poetry to lock dependency versions
  2. Remove nix stuff
  3. Add plain-Dockerfile

Resolves #288

@qqshfox qqshfox force-pushed the remove-nix branch 7 times, most recently from 5018477 to 8dab6c7 Compare March 1, 2021 15:04
@brettdh
Copy link

brettdh commented Mar 3, 2021

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.

Copy link
Contributor

@nejch nejch left a 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.

@qqshfox
Copy link
Contributor Author

qqshfox commented May 5, 2021

@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.

@nejch
Copy link
Contributor

nejch commented May 6, 2021

@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 :)

@qqshfox qqshfox added this to the 0.11.0 milestone Jun 2, 2021
@qqshfox qqshfox force-pushed the remove-nix branch 3 times, most recently from 9c10ea8 to 0452c4c Compare June 3, 2021 06:43
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.

Is Nix worth the complexity?
3 participants