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

Initial implementation #3

Merged
merged 71 commits into from
Mar 6, 2023
Merged

Initial implementation #3

merged 71 commits into from
Mar 6, 2023

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented Feb 3, 2023

Since cargo already treats them as semver version ranges (so this is effectively
a no-op), and this way it avoids conflicts/churn in this file from Dependabot PRs.
Since the vast majority do not need a changelog entry.
Since with the new beta Docker for macOS Rosetta feature the
build times on M1 have significantly improved.
Since it's testing an implementation detail of the functions runtime
and/or Pip, which isn't helpful to test in this buildpack.
Since we cache the pip cache only instead.
Since `ubuntu-latest` and `ubuntu-22.04` are now equivalent, since
their rollout of the change in default image version has now completed.
So it fits in the sidebar
@edmorley edmorley marked this pull request as ready for review February 24, 2023 16:10
@edmorley edmorley requested a review from a team February 28, 2023 10:12
Copy link
Member

@Malax Malax left a comment

Choose a reason for hiding this comment

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

The README could use some work, but this can easily be added later. That said, I think this is the new gold-standard for a Heroku languages-team CNB! I love the code, documentation and tests and wish my buildpack would look like this. It was a pleasure reviewing this! Awesome work dude! ✋🏻

I left a couple of nitpicky comments, but none are blocking at all.

tests/integration/python_version.rs Outdated Show resolved Hide resolved
tests/integration/pip.rs Show resolved Hide resolved
tests/integration/python_version.rs Show resolved Hide resolved
src/python_version.rs Show resolved Hide resolved
src/packaging_tool_versions.rs Show resolved Hide resolved
Copy link

@schneems schneems left a comment

Choose a reason for hiding this comment

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

I'm still working on finishing my review, but wanted to leave my pending comments.

src/utils.rs Show resolved Hide resolved
src/package_manager.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/layers/python.rs Show resolved Hide resolved
src/layers/python.rs Show resolved Hide resolved
src/layers/pip_cache.rs Show resolved Hide resolved
src/layers/pip_cache.rs Show resolved Hide resolved
src/layers/pip_dependencies.rs Show resolved Hide resolved
src/layers/python.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
Copy link
Member Author

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

@schneems Thank you for taking the time to review! :-)

src/layers/pip_cache.rs Show resolved Hide resolved
src/layers/pip_cache.rs Show resolved Hide resolved
src/layers/pip_dependencies.rs Show resolved Hide resolved
src/layers/python.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/layers/python.rs Show resolved Hide resolved
src/layers/python.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/package_manager.rs Show resolved Hide resolved
src/utils.rs Show resolved Hide resolved
Copy link
Member

@joshwlewis joshwlewis left a comment

Choose a reason for hiding this comment

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

Looks awesome! :shipit:

.github/workflows/ci.yml Show resolved Hide resolved
src/package_manager.rs Outdated Show resolved Hide resolved
src/utils.rs Show resolved Hide resolved
@edmorley edmorley dismissed schneems’s stale review March 6, 2023 11:20

All comments resolved

@edmorley edmorley enabled auto-merge (squash) March 6, 2023 11:29
@edmorley edmorley merged commit 11270fc into main Mar 6, 2023
@edmorley edmorley deleted the initial-implementation branch March 6, 2023 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants