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

Added support to build sdist of V2 module. #8711

Closed
wants to merge 8 commits into from

Conversation

arnaudsjs
Copy link
Contributor

@arnaudsjs arnaudsjs commented Feb 5, 2025

Description

Added support to build sdist of V2 module.

Part of #8111

  • Attached issue to pull request
  • Changelog entry
  • Type annotations are present
  • Code is clear and sufficiently documented
  • No (preventable) type errors (check using make mypy or make mypy-diff)
  • Sufficient test cases (reproduces the bug/tests the requested feature)
  • Correct, in line with design
  • End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
  • If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)

src/inmanta/moduletool.py Outdated Show resolved Hide resolved
src/inmanta/moduletool.py Outdated Show resolved Hide resolved
src/inmanta/moduletool.py Outdated Show resolved Hide resolved
@arnaudsjs arnaudsjs requested a review from sanderr February 7, 2025 14:43
Copy link
Contributor

@jptrindade jptrindade left a comment

Choose a reason for hiding this comment

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

I see that we are reusing Literal["wheel", "sdist"] a lot, should we create a type for it?

Comment on lines 144 to 145
# member_path = pathlib.Path(tar.getnames()[0])
# extract_dir = str(pathlib.Path(extract_dir).relative_to(member_path.parts[0]))
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems I forgot to push my latest commits.

Copy link
Contributor

@sanderr sanderr left a comment

Choose a reason for hiding this comment

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

Added comments

@arnaudsjs
Copy link
Contributor Author

arnaudsjs commented Feb 10, 2025

I see that we are reusing Literal["wheel", "sdist"] a lot, should we create a type for it?

@jptrindade Could you elaborate on this? What do you want to change it to? An Enum?

@arnaudsjs arnaudsjs requested a review from sanderr February 10, 2025 12:35
src/inmanta/moduletool.py Outdated Show resolved Hide resolved
src/inmanta/moduletool.py Show resolved Hide resolved
@arnaudsjs arnaudsjs requested a review from sanderr February 10, 2025 15:20
@arnaudsjs arnaudsjs added the merge-tool-ready This ticket is ready to be merged in label Feb 11, 2025
@inmantaci
Copy link
Contributor

Processing this pull request

inmantaci pushed a commit that referenced this pull request Feb 11, 2025
# Description

Added support to build sdist of V2 module.

Part of #8111

- [x] Attached issue to pull request
- [x] Changelog entry
- [x] Type annotations are present
- [x] Code is clear and sufficiently documented
- [x] No (preventable) type errors (check using make mypy or make mypy-diff)
- [x] Sufficient test cases (reproduces the bug/tests the requested feature)
- [x] Correct, in line with design
- [x] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
- [ ] ~~If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)~~
@inmantaci
Copy link
Contributor

Merged into branches master, iso8 in e9d13ab

@inmantaci inmantaci closed this Feb 11, 2025
@inmantaci inmantaci deleted the issue/8111-build-sdist-for-v2-module branch February 11, 2025 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-tool-ready This ticket is ready to be merged in
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants