-
Notifications
You must be signed in to change notification settings - Fork 313
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
Poetry Take 2 #1239
Poetry Take 2 #1239
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good however this PR is failing in CI. Also I would like to see some documentation added in the Contributions doc to at least tell users that they need to setup poetry to run builds and tests.
|
||
test-all: | ||
tox --parallel=auto | ||
poetry run tox --parallel=auto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to add some configs in pyproject.toml for tox?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever config currently exists for tox will work fine for now! I'm trying to limit the scope of this PR as much as possible.
file = "sceptre.template_handlers.file:File" | ||
s3 = "sceptre.template_handlers.s3:S3" | ||
http = "sceptre.template_handlers.http:Http" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it is meant to replace setup.py
. Is that True? If so, why is setup.py not removed in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is true. And I simply hadn't gotten to that just yet. I'm realizing I should have opened this as a draft PR for the time being!
[tool.poetry.scripts] | ||
sceptre = "sceptre.cli:cli" | ||
|
||
[tool.poetry.plugins."sceptre.hooks"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of the quotes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know, I asked myself the very same question when I was copying this over from the original PR I had 😆
I know I had a reason for them previously, but I couldn't tell you what that is now. My guess would be, something didn't work right without them. But I can do some experimentation to figure that out.
@@ -0,0 +1 @@ | |||
poetry.lock binary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically makes diffs easier to deal with. It's not absolutely necessary, and I'd be fine removing it, but diffs of this file are essentially useless since it is entirely machine generated.
The main thing this does is to tell git to not even bother trying to show a diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that... but then the counter-argument is that if somebody updates dependencies but forgets to update the lock file, I want to see on the PR that they haven't modified the lock file. Perhaps we can add a pre-commit check that the lock file is unchanged after an install or something? In cases with lock files, I don't actually care what the changes are, only that there is a change when someone has modified dependencies.
After looking this over and reading the comments so far, I realized it makes the most sense to convert this to a draft. I'm also thinking that it might be prudent to make some other changes before implementing this. I'm happy to take feedback on these proposed changes.
|
Unless there's a backport to 3.7 we can install, we cannot use that. That library was introduced in 3.8. It might be installable as a backport, in which case I'm fine doing that. But if we can't, then we shouldn't do that until 3.7 reaches end of life.
Regarding GH Actions vs. circleci... I don't really have a dog in that fight. I have no real experience in either. I don't care that much... though I don't see why a transition to Poetry would need to require a transition of CI/CD tool at the same time. I did have one other question: When Poetry creates a lock-file... that's just used for local development, right? It doesn't actually lock dependencies for those that install Sceptre as a library, right? There needs to be some "wiggle room" to install Sceptre into a virtual environment with other packages without entering a dependency hellscape. |
As a matter of fact, there is! https://pypi.org/project/importlib-metadata/
The main reasoning is that CircleCI, especially with Sceptre's current environment, is very hard to get right. You can see my previous attempt for some idea on just how much effort that takes. In contrast, a lot of what is being done manually with CircleCI just sorta "works" with GitHub Actions. For an example of a Poetry-based project that I set up a while back, see here: https://github.com/godaddy/tartufo/blob/main/.github/workflows/ci.yml And IIRC since then, GitHub Actions has even added some Poetry-specific setup into their So the tl;dr is, CircleCI is hard, GitHub Actions is easy, and if the transition is going to happen at some point I think it makes sense to do before Poetry, to avoid even more re-work.
The lock file is used for local development, and not for end-user installations, yes, that is correct. And there is no reason at all that wiggle room can't be present with Poetry! In fact you may notice, I had to do a bit of massaging of allowed versions already. For example: https://github.com/Sceptre/sceptre/pull/1239/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R60 Poetry has got all kinds of ability to specify those dependencies with whatever amount of wiggle room is necessary. |
As much as I would like to see this transition happen I don't envision it to happen anytime soon, or maybe at all. All of the repos in the GH Sceptre org is using circleci right now. I would hate to see us be in a state where we need to use two separate CI systems. In other words, unless there's someone that really want to put a lot of effort into moving Sceptre to GH actions I would just prefer to stay on one CI system. |
closing this PR in favor of #1323 |
After trying to update the pre-existing Poetry PR, and discovering numerous conflicts, I dediced it would be wise to start fresh. Luckily I was able to copy over much of my
work from previously, and enough things have been fixed now that this transition has been much easier this time around, so far.
I will need to go back through the other PR to find the full list of work that was necessary for this transition, but so far:
So I'd call this a pretty good start so far!
PR Checklist
[Resolve #issue-number]
.make test
) are passing.pre-commit run --all-files
).and description in grammatically correct, complete sentences.
Approver/Reviewer Checklist
Other Information
Guide to writing a good commit