feat: Install main dependency group only by default #182

Mehdi-H · 2023-07-30T19:14:40Z

Summary

This PR is linked to #180

Breaking change : default behavior is now to install only main dependency group from pyproject.toml
- Previous behavior: poetry installs every dependencies, even the ones not necessary for production, regardless of the dependency groups
Possibility to specify additional dependency groups with env variable BP_POETRY_INSTALL_WITH, mirroring the --with option from poetry install command
README.md is updated to describe this new behaviors
Unit and integration tests where added

Use Cases

Check #180 discussion for more details

Checklist

I have viewed, signed, and submitted the Contributor License Agreement.
I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
I have added an integration test, if necessary.
I have reviewed the styleguide for guidance on my code quality.
I'm happy with the commit history on this PR (I have rebased/squashed as needed).

linux-foundation-easycla · 2023-07-30T19:14:44Z

❌ The email address for the commit (be22893) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

* Choose additionnal dependency groups to install via BP_POETRY_INSTALL_WITH env variable

robdimsdale · 2023-07-30T20:18:12Z

Thanks @Mehdi-H - could you sign the CLA? The bot above has a link. You can either sign as an individual or as part of a corporate CLA.

robdimsdale

Thank you for submitting this! This generally looks good. I have a few comments/suggestions/questions inline, and once we address those I think we'll be good to merge this.

robdimsdale · 2023-07-30T20:20:18Z

integration/init_test.go

 	SetDefaultEventuallyTimeout(30 * time.Second)

 	suite := spec.New("Integration", spec.Report(report.Terminal{}))
-	suite("Default", testDefault, spec.Parallel())


I see we removed the spec.Parallel() - was that intentional? Did you observe the tests failing with that present?

robdimsdale · 2023-07-30T20:23:25Z

integration/build_with_dependency_group_test.go

+				MatchRegexp(fmt.Sprintf(
+					"    Running 'POETRY_CACHE_DIR=/layers/%s/cache POETRY_VIRTUALENVS_PATH=/layers/%s/poetry-venv poetry install --with dev'",
+					strings.ReplaceAll(buildpackInfo.Buildpack.ID, "/", "_"),
+					strings.ReplaceAll(buildpackInfo.Buildpack.ID, "/", "_"),


I don't think we need this strings.ReplaceAll twice - the second invocation should be redundant.

robdimsdale · 2023-07-30T20:24:41Z

integration/build_with_dependency_group_test.go

+				MatchRegexp(fmt.Sprintf(`    PATH                    -> "/layers/%s/poetry-venv/default-app-.*-py\d+\.\d+/bin:\$PATH"`, strings.ReplaceAll(buildpackInfo.Buildpack.ID, "/", "_"))),
+				MatchRegexp(fmt.Sprintf(`    POETRY_VIRTUALENVS_PATH -> "/layers/%s/poetry-venv"`, strings.ReplaceAll(buildpackInfo.Buildpack.ID, "/", "_"))),
+				MatchRegexp(fmt.Sprintf(`    PYTHONPATH              -> "/layers/%s/poetry-venv/default-app-.*-py\d+\.\d+/lib/python\d+\.\d+/site-packages:\$PYTHONPATH"`, strings.ReplaceAll(buildpackInfo.Buildpack.ID, "/", "_"))),
+				"",


Can you remove the "" and replace it with another invocation of Expect(logs).To(ContainLines(...))?

We've found that asserting on empty log lines is brittle, because sometimes extra lines get inserted due to environment issues (deprecation warnings, etc).

A second invocation of ContainLines() will continue to preserve the assertions on the line ordering.

robdimsdale · 2023-07-30T20:28:45Z

integration/build_with_dependency_group_test.go

+				// check an SBOM file to make sure it has an entry for a poetry dependency
+				contents, err := os.ReadFile(filepath.Join(sbomDir, "sbom", "launch", strings.ReplaceAll(buildpackInfo.Buildpack.ID, "/", "_"), "poetry-venv", "sbom.cdx.json"))
+				Expect(err).NotTo(HaveOccurred())
+				Expect(string(contents)).To(ContainSubstring(`"name": "flask"`))


For this test, could you also add the environment variable: WithEnv(map[string]string{"BP_POETRY_INSTALL_WITH": "dev"}). and could you also add an assertion that the SBOM contains a dev dependency?

Similarly, could you add an assertion to the default_test to assert that the SBOM does not contain a dev dependency?

robdimsdale · 2023-07-30T20:30:57Z

Also, as discussed in #180 - although this is technically a breaking change, I don't think it will break any users in practice, as I don't believe users are expecting their apps to contain dependencies other than the main group. The fact that they were, and now will be removing them by default, should be invisible to users.

As such, I'm labeling this as: semver:minor

robdimsdale · 2023-09-07T21:40:33Z

@Mehdi-H as far as the CLA, I think part of the issue is that your commit was made from an email that isn't associated with your github account. The links in the CLA bot's comment above should help.

radoshi · 2024-04-19T21:20:26Z

Hi @Mehdi-H @robdimsdale ,

We would like to see this issue fixed as well. I think Mehdi has done all of the heavy lifting here but this PR seems to have fallen off.

Would y'all have any objections if I picked it up and finished it? Here is a draft PR with all the review comments addressed. It passes unit and integration tests.
#347

Thank you,
-Rushabh

TheSuperiorStanislav · 2024-09-04T09:16:53Z

Hi @Mehdi-H @robdimsdale any progress on this one?

Mehdi-H requested a review from a team as a code owner July 30, 2023 19:14

Mehdi-H mentioned this pull request Jul 30, 2023

[Question ❓] How to install only "root/run" dependencies and ignore other groups dependencies #180

Closed

feat: Install main dependency group only by default

be22893

* Choose additionnal dependency groups to install via BP_POETRY_INSTALL_WITH env variable

Mehdi-H force-pushed the install-dependencies-with-group branch from ca25f4c to be22893 Compare July 30, 2023 19:26

robdimsdale added the semver:minor A change requiring a minor version bump label Jul 30, 2023

robdimsdale reviewed Jul 30, 2023

View reviewed changes

radoshi mentioned this pull request Apr 19, 2024

feat: Install main dependency group only by default #347

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Install main dependency group only by default #182

feat: Install main dependency group only by default #182

Uh oh!

Mehdi-H commented Jul 30, 2023 •

edited

Loading

Uh oh!

linux-foundation-easycla bot commented Jul 30, 2023 •

edited

Loading

Uh oh!

robdimsdale commented Jul 30, 2023

Uh oh!

robdimsdale left a comment

Uh oh!

robdimsdale Jul 30, 2023

Uh oh!

robdimsdale Jul 30, 2023

Uh oh!

robdimsdale Jul 30, 2023

Uh oh!

robdimsdale Jul 30, 2023

Uh oh!

robdimsdale commented Jul 30, 2023 •

edited

Loading

Uh oh!

robdimsdale commented Sep 7, 2023

Uh oh!

radoshi commented Apr 19, 2024

Uh oh!

TheSuperiorStanislav commented Sep 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: Install main dependency group only by default #182

Are you sure you want to change the base?

feat: Install main dependency group only by default #182

Uh oh!

Conversation

Mehdi-H commented Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Use Cases

Checklist

Uh oh!

linux-foundation-easycla bot commented Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robdimsdale commented Jul 30, 2023

Uh oh!

robdimsdale left a comment

Choose a reason for hiding this comment

Uh oh!

robdimsdale Jul 30, 2023

Choose a reason for hiding this comment

Uh oh!

robdimsdale Jul 30, 2023

Choose a reason for hiding this comment

Uh oh!

robdimsdale Jul 30, 2023

Choose a reason for hiding this comment

Uh oh!

robdimsdale Jul 30, 2023

Choose a reason for hiding this comment

Uh oh!

robdimsdale commented Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robdimsdale commented Sep 7, 2023

Uh oh!

radoshi commented Apr 19, 2024

Uh oh!

TheSuperiorStanislav commented Sep 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mehdi-H commented Jul 30, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jul 30, 2023 •

edited

Loading

robdimsdale commented Jul 30, 2023 •

edited

Loading