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

Move debugtoolbar and silk to regular #3076

Merged

Conversation

jxltom
Copy link
Contributor

@jxltom jxltom commented Oct 9, 2018

As decided here #3042 (comment), packages related with codebase directly are moved to regular requirements.

It is waiting for #3044 to be merged first, otherwise there will be conflicts since they are related with requirements files.

Since these packages are moved to regular requirements, --dev in building docker image is unnecessary.

Fixes: #3042

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. The changes are tested.
  6. The code is documented (docstrings, project documentation).
  7. GraphQL schema and type definitions are up to date.

@jxltom
Copy link
Contributor Author

jxltom commented Oct 9, 2018

BTW, there some issues with recent pipenv version (2018.6.25/2018.7.1). If there are any issues relating with locking, please use 11.9.0.

Since pypa/pipenv#2940 and pypa/pipenv#2935, which fixed amounts of parsing and locking issues, are merged, I'm sure the next release of pipenv, which will be soon I think, will be a stable one.

pip install https://github.com/pypa/pipenv/archive/master.zip also works if anyone would like to try the latest pipenv.

@Pacu2
Copy link
Contributor

Pacu2 commented Oct 9, 2018

I don't think we wanted to move Silk do the regular requirements, only debug toolbar is used if one uses DEBUG mode, Silk is the matter of choice and needs to be enabled manually.

@jxltom
Copy link
Contributor Author

jxltom commented Oct 9, 2018

If that is the case, #3039 may still be reasonable. Someone may want to enable silk in docker. If it is put in dev packages, it can never be enabled in docker because it is not installed.

Also, in heroku, it won't install dev packages by default. So one can never enable silk in server deployed by heroku since it won't be installed in heroku at all, if it is put in dev packages.

@Pacu2
Copy link
Contributor

Pacu2 commented Oct 9, 2018

To be honest I doubt that Silk should ever make it to the production/live environment.

@jxltom
Copy link
Contributor Author

jxltom commented Oct 9, 2018

Actually I think even debugtoolbar should never be used in production server. And that is why I comment here #3043 (comment).

But it looks like #3043 (comment) think it should be regular packages even it is controlled by environment variables.

I'm fine with installing these kind of packages in production server as long as it is under control and environment variables for enabling them are never be enabled in production server.

I'm also fine with no installing for these packages in prod server at all. But it looks like this will break saleor's deployment since by default DEBUG is true in saleor.

But it confuses me if debugtoolbar stays in regular package as silk stays in dev package. Maybe they should be stay together?

@Pacu2
Copy link
Contributor

Pacu2 commented Oct 9, 2018

@jxltom We've discussed it internally, all dependencies (even optional like Debug toolbar or Silk) should live inside of the regular requirements.

dev_requirements.txt are just the tools that are helping developers in doing their jobs, like linters etc.

@jxltom
Copy link
Contributor Author

jxltom commented Oct 9, 2018

Cool. A discussion to clarify this is very helpful.

@maarcingebala maarcingebala merged commit ca013af into saleor:master Oct 9, 2018
@jxltom jxltom deleted the move-debugtoolbar-and-silk-to-regular branch October 9, 2018 08:19
@maarcingebala
Copy link
Member

Build on CircleCI failed due to a missing pytest dependency. I think we should move it to regular dependencies as well since it is required by the test suite, along with all of its plugins.

https://circleci.com/gh/mirumee/saleor/3605

@jxltom
Copy link
Contributor Author

jxltom commented Oct 9, 2018

@maarcingebala Oh, I didn't know there are also tests are running in docker containers. https://github.com/mirumee/saleor/pull/3076/files#diff-3254677a7917c6c01f55212f86c57fbf is should be reverted.

RUN pipenv install --system --deploy --dev

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.

3 participants