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 /opt/graphite prefix and use setuptools #2409

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

Conversation

piotr1212
Copy link
Member

See graphite-project/carbon#835 for reasoning

@piotr1212 piotr1212 requested a review from deniszh January 8, 2019 12:43
@codecov-io
Copy link

codecov-io commented Jan 8, 2019

Codecov Report

Merging #2409 into master will decrease coverage by 0.02%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2409      +/-   ##
==========================================
- Coverage   80.26%   80.24%   -0.03%     
==========================================
  Files          87       87              
  Lines        9323     9333      +10     
  Branches     1983     1984       +1     
==========================================
+ Hits         7483     7489       +6     
+ Misses       1558     1557       -1     
- Partials      282      287       +5     
Impacted Files Coverage Δ
webapp/graphite/logger.py 87.03% <69.23%> (-5.83%) ⬇️
webapp/graphite/settings.py 74.75% <100.00%> (-0.25%) ⬇️
webapp/graphite/dashboard/views.py 98.51% <0.00%> (-0.75%) ⬇️
webapp/graphite/wsgi.py 40.62% <0.00%> (ø)
webapp/graphite/app_settings.py 70.96% <0.00%> (ø)
webapp/graphite/tags/base.py 91.79% <0.00%> (+1.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94aad8f...42cc6d2. Read the comment docs.

@piotr1212
Copy link
Member Author

piotr1212 commented Feb 2, 2019

I'm not finished yet but made some changes to the docs which I would like to get some feedback for. I've tried to simplify the docs, IMO there were too much separate pages which you had to jump back and forward to.

I've changed the default install (settings.py) so that running collectstatic is not needed. The static files can be served directly from the app with whitenoise. Serving from whitenoise should be fast enough for most installations. this eliminates the need for configuring the static dir in the webserver (simplifies installation). From what I've read the whole purpose of collectstatic is for organisations which run multiple django apps and have separated their static files from code (in repo), so that they can update static files without having to deploy code and vice versa. As graphite's static files haven't changed in years and they are in the code repo I don't see a point to require collectstatic in the default install. Users can still run collectstatic if they want/need.

@piotr1212 piotr1212 force-pushed the setuptools branch 2 times, most recently from 5b1cd5e to 1ba4da5 Compare February 7, 2019 10:35
setup.py Outdated Show resolved Hide resolved
@ploxiln
Copy link
Contributor

ploxiln commented Feb 13, 2019

I think a rebase went a bit wrong, you ended up with a copy of the commit "fix dashboard graph metric list icon paths with URL_PREFIX" from the master branch
in master: 0a037db
in this branch: 1ba4da5

Don't install in /opt/graphite anymore, this is just strange and causes nothing
but trouble. Setuptools does not support it either.

Switch to setuptools to get rid of following warnings:
UserWarning: Unknown distribution option: 'install_requires'
UserWarning: Unknown distribution option: 'long_description_content_type'
Synthesize is not being updated anymore (latest version is 1.0.2). Removing
it from the docs to not confuse users. And we have an official docker image now.
The GRAPHITE_ROOT was to be set to the parent directory of the path where
graphite-web is installed. The `storage`, `static` and `content` directories are
set to be dubdirs of GRAPHITE_ROOT.

If installing Graphite in the regular Python path's this would make the directories
look something like `/usr/lib/python3.7/{storage,static,conf}`. Which is probably not
what the user wants.

This sets the default to `sys.prefix`.

sys.prefix
Installing the static assets in the project directory instead of an external directory
make installation easier.

Add whitenoise to requires in setup.py so that the static assets are automatically (without
user action) served after installation.

Remove package_data from setup.py, you should use package_data or MANIFEST.in to specify
which non-python files have to be installed, not both.
…_settings.py

Fixes dashboards which were broken with last commit
@piotr1212
Copy link
Member Author

I think a rebase went a bit wrong, you ended up with a copy of the commit "fix dashboard graph metric list icon paths with URL_PREFIX" from the master branch
in master: 0a037db
in this branch: 1ba4da5

I think I merged instead of rebased. Anyway, cleaned up now.


Carbonate
^^^^^^^^^
kA None
Copy link
Contributor

Choose a reason for hiding this comment

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

very minor - this "kA" looks unintentional

@ploxiln
Copy link
Contributor

ploxiln commented Feb 20, 2019

All looks good to me.

I had another thought about storage dirs ... I think the original idea behind using /opt/graphite is those storage and log dirs, which would be awkward in the python site-packages directory. Maybe the thing to do is divorce them from the graphite application root, and default to /opt/graphite/storage and /opt/graphite/log regardless of the install prefix? And not mention them in setup.py at all? I suppose the downside is they would not be created by install. Just an idea - I suppose I always customize these dirs anyway.

@piotr1212
Copy link
Member Author

good point. I'll have a look. But busy at the moment with more higher prio stuff.

@stale
Copy link

stale bot commented Apr 13, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 13, 2020
@ploxiln
Copy link
Contributor

ploxiln commented Apr 13, 2020

This is a big painful change, but I think it's still relatively important. Just sayin' to appease the stale bot :)

@adamboutcher
Copy link

If this will fix the pip install, can we get it merged?

@deniszh
Copy link
Member

deniszh commented Jul 19, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants