Repository navigation
chore(template): bundle e2b-charts in the template instead of PyPI - #363
Conversation
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It changes how the release pipeline ships the chart extractor, and it removes a required branch-protection check, so a code owner should still look at it.
What was reviewed:
- Template packaging:
.copy("charts", "charts")and./chartsin requirements.txt both resolve inside the template/ file context, and the copy runs before pip install. - release.yml gating: build-docker-image and build-template now depend only on
preflight.outputs.template. No job still refers to charts-release or its outputs. - New tests:
test_blank.pyuses the existingclientfixture andtest_grid_lines.pyuses therun_chartfixture from tests/charts/conftest.py.
Extended reasoning...
The change moves the e2b_charts sources into template/charts/ unchanged and installs them as a local path requirement. It deletes the package's PyPI publish job, its unit tests and the charts_tests.yml workflow, which provides a required status check. It also removes the package from the pnpm workspace and lockfile. It touches no injection, auth or data-exposure surface; the only credential-related change is that the PyPI trusted-publish job is gone. CODEOWNERS assigns every path to a single owner, and the change alters how releases work (chart changes now need a template changeset), so a human should review it instead of an automatic approval.
71db37c to
5c11238
Compare
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
5c11238 to
c97ec63
Compare
Summary
Ships the chart data extractor from source with the template so
e2b-chartscan be unpublished from PyPI, and tests it only through the sandbox suite.chart_data_extractor/moves totemplate/charts/. It is now a plainuv_buildpackage with no lockfile or dev group. It has to live insidetemplate/because that directory is the template's file context.startup_scripts/0002_data.py(from e2b_charts import chart_figure_to_dict) is unchanged.requires = ["uv_build==0.13.4"](previously>=0.10.0,<0.11.0). pip builds./chartsin an isolated env during the image build, so this keeps a newuv_buildrelease from being pulled in automatically.charts_tests.yml.tests/chartsalready covered every figure case against a real sandbox. The two gaps are added there:test_blank.py: an empty figure giveschart == {}.test_grid_lines.py:axhline,axvlineandgridlines are not chart elements, which coversis_grid_line.charts-releasejob (PyPI trusted publish) and thesed e2b_charts==VERSIONpinning inrelease.ymlandbuild_test_template.yml. Template builds now run only for@e2b/code-interpreter-templatechangesets, so chart changes need a template changeset.@e2b/data-extractoris removed from the pnpm workspace; itspackage.jsononly versioned the PyPI release. ItsCHANGELOG.mdis removed too.template/.dockerignorekeeps a localcharts/.venvand caches out of the build context.Branch protection:
charts-tests / Chart Data Extractor - Build and testwas a required check and no longer exists. Remove it from the required checks.Link to Devin session: https://app.devin.ai/sessions/89cd9e9b64e74c8db84e1e74794c1455
Open in Devin Desktop: https://app.devin.ai/desktop/session/89cd9e9b64e74c8db84e1e74794c1455?variant=devin
Requested by: @mishushakov