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

feat: Allow defining direct path to pyproject.toml #525

Merged
merged 8 commits into from
Jan 17, 2024

Conversation

n8felton
Copy link
Contributor

@n8felton n8felton commented Jan 4, 2024

Description

This change allows you to define the location of a pyproject.toml file, rather than relying on the discovery of the file in a given directory.

Example 1

  source_path = [
    "src",
    "pyproject.toml"
  ]

Example 2

  source_path = [
    "src",
    {
      path = "pyproject.toml",
      poetry_install = true
    }
  ]

Motivation and Context

Previously, discovery and execution of poetry related commands required discovery of the pyproject.toml, poetry.lock, and poetry.toml in a given directory, which required the entire directory to be included in the resulting Zip file, which is not always desired. This PR allows you to define the direct path to the pyproject.toml without needing to rely on it being discovered.

Addition context in #524

Breaking Changes

In my testing, this does not cause any breaking changes with existing poetry deployments.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects

I have also validated these changes against the existing package_dir_poetry module.

  • I have executed pre-commit run -a on my pull request

Fixes #524

@n8felton n8felton changed the title Allow defining direct path to pyproject.toml fix: Allow defining direct path to pyproject.toml Jan 4, 2024
@mar-pan
Copy link

mar-pan commented Jan 8, 2024

Hello module devs. Can we have this reviewed and merged? I'm facing issue which this one will solve

@antonbabenko
Copy link
Member

@pdecat Could you please take a look at this one?

@pdecat
Copy link
Contributor

pdecat commented Jan 12, 2024

Hi, I left some comments in the issue. Right now, I believe changes should not be needed to make this work.

Copy link
Contributor

@pdecat pdecat left a comment

Choose a reason for hiding this comment

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

I guess I understand the changes, looks good overall. Left a few comments, PTAL.

I'll test this ASAP.

package.py Show resolved Hide resolved
poetry_lock_target_file = copy_file_to_target(poetry_lock_file, temp_dir)
else:
poetry_lock_target_file = None

if os.path.isfile(poetry_toml_file):
log.info("Using poetry configuration file: %s", poetry_lock_file)
log.info("Using poetry.toml configuration file: %s", poetry_toml_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of duplicates the poetry.toml in the output, doesn't it?

Suggested change
log.info("Using poetry.toml configuration file: %s", poetry_toml_file)
log.info("Using poetry toml configuration file: %s", poetry_toml_file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to be consistent with the poetry.lock file output above, so if neither are desired, it can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The larger change and bug fix was having the output point to poetry_toml_file instead of poetry_lock_file

Copy link
Contributor

Choose a reason for hiding this comment

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

The larger change and bug fix was having the output point to poetry_toml_file instead of poetry_lock_file

Yeah, noticed that part 👍

package.py Outdated Show resolved Hide resolved
package.py Outdated Show resolved Hide resolved
package.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pdecat pdecat left a comment

Choose a reason for hiding this comment

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

While trying the new examples, it failed because python3.9-app-src-poetry does not exist.

Did you mean python3.9-app-poetry, or did you forget to add that directory?

Edit: I guess it is the latter, and it should contain an src/ sub-directory.

examples/build-package/main.tf Show resolved Hide resolved
examples/build-package/main.tf Show resolved Hide resolved
examples/build-package/main.tf Show resolved Hide resolved
examples/build-package/main.tf Show resolved Hide resolved
examples/build-package/main.tf Show resolved Hide resolved
Copy link
Contributor

@pdecat pdecat left a comment

Choose a reason for hiding this comment

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

Please add the missing python3.9-app-src-poetry test fixture, and use different values for artifacts_dir.

examples/build-package/main.tf Outdated Show resolved Hide resolved
examples/build-package/main.tf Outdated Show resolved Hide resolved
Copy link
Contributor

@pdecat pdecat left a comment

Choose a reason for hiding this comment

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

A few issues with the examples remain.

Copy link
Contributor

@pdecat pdecat left a comment

Choose a reason for hiding this comment

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

Finally tested this against some existing stacks, does not break anything.

LGTM

@n8felton
Copy link
Contributor Author

n8felton commented Jan 16, 2024

🎉

Thank you for working through this PR with me, I appreciate the timely back and forth to get things answered and resolved. 🙇

@antonbabenko - I believe this is ready for your review.

@antonbabenko antonbabenko changed the title fix: Allow defining direct path to pyproject.toml feat: Allow defining direct path to pyproject.toml Jan 17, 2024
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

LGTM

@antonbabenko antonbabenko merged commit d33b722 into terraform-aws-modules:master Jan 17, 2024
30 checks passed
antonbabenko pushed a commit that referenced this pull request Jan 17, 2024
## [6.8.0](v6.7.1...v6.8.0) (2024-01-17)

### Features

* Allow defining direct path to pyproject.toml ([#525](#525)) ([d33b722](d33b722))
@antonbabenko
Copy link
Member

This PR is included in version 6.8.0 🎉

@antonbabenko
Copy link
Member

Awesome! Thanks to both of you for the cooperation & review!

@n8felton n8felton deleted the poetry_path branch January 17, 2024 14:59
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to use Poetry with src directory but with poetry.lock and pyproject.toml at root.
4 participants