-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Install ext-theme
from requirements.txt
#12264
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
base: main
Are you sure you want to change the base?
Conversation
Move this Python package into our requirements workflow. Closes #12140
Won't this affect production? We will be installing the latest version available on main instead of the version from the release. Also, does this still allow us to hot reload locally? Like the changes from the local copy are taken into effect when running .org or .com locally |
Yea, we should probably at least use a |
I think both are correct here. We were installing from I kept the installation for development inside Please, take another look and let me know. By the way, the PR that removes the Salt states is at https://github.com/readthedocs/readthedocs-ops/pull/1662 |
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.
This seems reasonable to me. Just to confirm, this doesn't change how we do it in Docker, where it's using a local mount, right?
Yeah, we are not changing that. |
@@ -9,3 +9,6 @@ structlog-sentry | |||
newrelic==10.7.0 | |||
|
|||
ipython | |||
|
|||
# Theme for the dashboard (install from rel) | |||
readthedocsext-theme @ git+https://github.com/readthedocs/ext-theme@rel |
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.
What about .com? We need to install from relcorp there.
This sounds like it's more complex than we thought. I think we can simplify it if we don't deploy ext-theme from Maybe it's just a non-issue and we keep it as it's currently. Also, I think @agjohnson was suggesting to move the ext-theme into the .org repository (correct me if I'm wrong). In that case, we won't need anything else here. |
So I had started down this path, but hit some of the same here:
I would explicitly check that this works. I hit this trouble when I tried this, package installation required me to rebuild or call pip after changes to template files. I'd consider either case a strong no go. I want to preserve being able to edit a template and the changes show in < 5 seconds, namely.
It would be helpful to be able to run separate versions of ext-theme, but I don't think we want to enable this pattern much either. I'd love to version ext-theme, and it would be helpful to target individual versions for business/community but probably not strictly required either.
I prefer to have everything separate. Tests are isolated, granular, and faster, and I this repository isn't burdened by all of the baggage of the application repo. It also helps having an isolated tracker and milestones. If we end up being blocked by this it's something we can consider though. So, overall, I would love to be able to have a versioned package repository that hot reloads correctly. It that means we lose the rel/relcorp pattern on ext-theme, I think that trade off is fine to make. |
This PR is not changing how we install ext-theme in Docker. So, hot reloads will keep working. On the other hand, I'm not sure that having a versioned package repository adds a lot of value here and I think it just adds more complexity. My idea with this PR was to simplify the process while keeping it the same without designing a new release pattern (version the repository and/or create a Python package). If we want to follow that path, we can do it in a different task. The only blocker that I found on this PR is that we can install community from |
Versioning ext-theme, or more packages in general, is a farther out want for me. The value I see there is having versions that we use outside release. We only interact with rel/relcorp immediately after they are created and we get tripped up by changes we didn't realize were missing in rel/relcorp rather frequently. I'm noting this as I'd like to move in this general direction, but it doesn't need to happen now. To support a rel/relcorp split, does pip tools support the same env var expansion that pip does? Also looking forward, we'll use We don't really use the rel/relcorp split on the templates either way, the templates should always be the same code on both. These branches are really just used as a workaround to tagging commits on this repo (and ext and others too). If we want to try reducing the rel/relcorp pattern, I'm not opposed. |
The main reason we have rel/relcorp is so we can have a production "point in time" that we can hotfix against even after |
I think environment variables is a good try for now so we keep rel/relcorp around and we don't have to change our deploy process too much 👍🏼 . In theory, it's supported so I will give it a try. |
👍🏼 |
Yeah I'm still talking about making artifacts used for release too. Actually, this is sort of my point too, rel/relcorp is a slight hack to just emulate tagging. Tagging/versioning gets us the same result except we'd also use these tags in local development.
So I'm only talking about versioning here and maybe libraries like The application is indeed harder, I'm not sure how much we can change that pattern yet. Anyways, this is all just future talk. The longer term aim here is to share the artifacts we use for release with the artifacts we use for development -- or more specifically, rel/relcorp are created right before we use them and we don't test what is in them until it's on a server in prod.
Env vars seems like the best immediate option. We knew pip supports env vars, the big question is if pip-tools or uv would. Hopefully this is an easy path forward for now though. |
Move this Python package into our requirements workflow.
Closes #12140