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

Break out Addons injection module to own repository #226

Open
1 of 5 tasks
agjohnson opened this issue Oct 21, 2024 · 2 comments
Open
1 of 5 tasks

Break out Addons injection module to own repository #226

agjohnson opened this issue Oct 21, 2024 · 2 comments
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code

Comments

@agjohnson
Copy link
Contributor

agjohnson commented Oct 21, 2024

While we use the module directly here in local development, it feels very hidden and disconnected from the rest of Addons. Users looking to report issues with the injection are probably looking at our main repository or the Addons repository. This module might be better authored and maintained in one of those locations instead.

I encountered this wanting to add tests to the script

It seems like we'd need:

  • Move to Addons repo or separate repo entirely?
  • Community/commercial Dockerfile checks out or reuses ${PWD}/${RTDDEV_PATH_ADDONS:-../addons}:/usr/src/app/checkouts/addons similar to other repo reuse
  • Docker compose builds addons inject wrangler image from Git HTTPS URL
  • Wrangler uses configuration file instead of hardcoded configuration in docker compose
  • CircleCI config
@agjohnson agjohnson added Improvement Minor improvement to code Accepted Accepted issue on our roadmap labels Oct 21, 2024
@github-project-automation github-project-automation bot moved this to Planned in 📍Roadmap Oct 21, 2024
@agjohnson agjohnson changed the title Move Addons injection script Break out Addons injection module to own repository Oct 22, 2024
agjohnson added a commit that referenced this issue Oct 23, 2024
We should still figure out moving these files in

- #226
@humitos
Copy link
Member

humitos commented Oct 28, 2024

I'm not opposed to move it to its own repository, but I wanted to note the work required won't be trivial. I've put it in common/ because it's the way we currently have to share code between multiple repositories. In this particular case, we need to share it with readthedocs.org, readthedocs-corporate and also readthedocs-ops.

I'd propose starting by adding whatever is needed to run/write tests into this common repository first and grow from there if it's required, instead of the other way around.

Users looking to report issues with the injection are probably looking at our main repository or the Addons repository

I'm not worried about this. I see this script more as internal ops than user exposed. I'm fine if it's not super discoverable. In fact, it was originally inside ops but I had to move it because of the sharing between repositories needs.

@agjohnson
Copy link
Contributor Author

agjohnson commented Oct 28, 2024

because it's the way we currently have to share code between multiple repositories

We also check out ext and ext-theme to their own repos during the docker image build instead of using submodules. I also like this pattern and prefer it over the common submodule.

I'd propose starting by adding whatever is needed to run/write tests into this common repository first and grow from there if it's required, instead of the other way around.

This is already happening in #227

agjohnson added a commit that referenced this issue Oct 29, 2024
* Move addons injection files into package structure

We should still figure out moving these files in

- #226

* Add supplemental files to support updated addons injection module

* Update injection script

* Fix issues with response return order

If `Response.clone()` is used without `await` the resulting instance has
missing attributes, like `url = null`. The docs do not indicate that
`clone()` is async however, so it's not clear why this is required.

With this, and more careful response instance usage, most of the cases
of dangling, unused, cloned responses have been removed. Now, any
response that is used is cloned right as we use it and evaluate the
body. This is important, as `response.body` is ultimately what causes
the request to buffer.

* Update throw debug header

* Add more comments on response cloning

* Bump up conditional for debug error throw

* Test more response header passthrough

* Add test for build.commands builds

* Add path to wrangler build context

* Fix wrangler path in docker compose

* Increase logging on wrangler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
Status: Planned
Development

No branches or pull requests

2 participants