graphite-web icon indicating copy to clipboard operation
graphite-web copied to clipboard

Remove /opt/graphite prefix and use setuptools

Open piotr1212 opened this issue 5 years ago • 10 comments

See https://github.com/graphite-project/carbon/pull/835 for reasoning

piotr1212 avatar Jan 08 '19 12:01 piotr1212

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%) :arrow_down:
webapp/graphite/settings.py 74.75% <100.00%> (-0.25%) :arrow_down:
webapp/graphite/dashboard/views.py 98.51% <0.00%> (-0.75%) :arrow_down:
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%) :arrow_up:

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.

codecov-io avatar Jan 08 '19 12:01 codecov-io

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 avatar Feb 02 '19 20:02 piotr1212

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: 0a037db4b2d864734e14dd6302bc71194f53e8d3 in this branch: 1ba4da55c08035cccfcdaae2220f8d384dbd1929

ploxiln avatar Feb 13 '19 04:02 ploxiln

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.

piotr1212 avatar Feb 20 '19 17:02 piotr1212

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.

ploxiln avatar Feb 20 '19 20:02 ploxiln

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

piotr1212 avatar Feb 20 '19 21:02 piotr1212

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[bot] avatar Apr 13 '20 20:04 stale[bot]

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

ploxiln avatar Apr 13 '20 21:04 ploxiln

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

adamboutcher avatar Jul 19 '23 16:07 adamboutcher

Unfortunately, it's not that easy. That would break backward compatibility, and need more changes in carbon. Also, not sure if that fix issue too :/

ср, 19 июл. 2023 г., 18:36 Adam @.***>:

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

— Reply to this email directly, view it on GitHub https://github.com/graphite-project/graphite-web/pull/2409#issuecomment-1642409417, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJLTVRNEHAUAT6W7NWQH7LXRAEH3ANCNFSM4GOVTH5Q . You are receiving this because your review was requested.Message ID: @.***>

deniszh avatar Jul 19 '23 17:07 deniszh