#453 Add command to check poetry.lock freshness#1954
#453 Add command to check poetry.lock freshness#1954finswimmer merged 1 commit intopython-poetry:masterfrom
Conversation
finswimmer
left a comment
There was a problem hiding this comment.
Nice 👍
Because this is a new feature @sdispater will decide if this get included.
Thanks a lot for your contribution!
poetry/console/commands/lock.py
Outdated
| installer.lock() | ||
|
|
||
| return installer.run() | ||
| return installer.run() |
There was a problem hiding this comment.
As there is already a return in the previous if, I think it would be better to remove this else. It will help with further additions to the command.
There was a problem hiding this comment.
Just to make sure I understand, you would prefer the following?
| return installer.run() | |
| if self.option("check"): | |
| return (expr) | |
| installer.lock() | |
| return installer.run() |
|
Hmm, sorry for the comment I deleted, I wasn't aware this was actually related to #453 , which I posted! :-) So, there's a lot of checking I want to do in one easy place:
The latter might seem paranoid, but with venv in a project it's easy sometimes to forget to use poetry and just pip install something. Also, I often work with a flaky network connection, and if poetry is struggling, I frequently need to Ctrl-C, and I'm never sure what state the virtualenv has been left in. |
|
Would this flag also check things like What's the status of this PR? I'd love to have it! |
|
Hi, any update on this really nice PR? |
|
Would love this feature! |
|
Any reason this hasn't been merged? |
|
I'd love to see this feature as well, anything the community can do to help it along? |
|
@cliebBS any chance you can get this rebased and conflicts resolved please? Additionally, would be great to have a test case for this one. |
|
I've fixed the merge conflict, the only test failure was a network error on one of the FreeBSD builds during test setup, unrelated to my change. I can't figure out how to rerun the test, as I'm sure it would pass without issue. As for the tests that you've asked for, I'm not sure how to setup a test for the I have tested my changes manually by first running |
|
Whats blocking this PR so far? |
|
@finswimmer I've merged master and rewritten my test to match the new test pattern used in the other test in |
finswimmer
left a comment
There was a problem hiding this comment.
Thanks a lot for your contribution 👍 Looks good to me. Just one minor question about a test.
tests/console/commands/test_lock.py
Outdated
|
|
||
|
|
||
| @pytest.mark.skipif( | ||
| sys.platform == "win32", reason="does not work on windows under ci environments" |
There was a problem hiding this comment.
What's happening under windows? Do you know why?
There was a problem hiding this comment.
That was many moons ago and I've totally forgotten why I added it. I'm going to try removing it to see if the test works on Windows now.
There was a problem hiding this comment.
Hey, wha'd'ya know? Totally unnecessary anymore :)
There was a problem hiding this comment.
Perfect 👍 Could you please add a test for a non outdated lock file as well?
There was a problem hiding this comment.
I've added a new test and refactored the testing a bit. I keep getting failures on the FreeBSD/Py36 tests while installing dependencies, though. All of the other Py36 testing is passing, so I can only figure that something is broken on that test runner.
There was a problem hiding this comment.
Ok, today tests are all passing, including the new test :)
6bcfb22 to
b6bd0bb
Compare
|
github is telling me it cannot merge due to conflicts, but it seems to be a secrets what conflicts 🤔 Can you please rebase once more? |
|
I just tried and there are no new commits on upstream/master to merge in. Also, I'm seeing on the PR that there are no confiicts 🤔 |
|
I think an actual @cliebBS as your individual commits don't seem to carry much meaning (beyond just "these are the steps in which I tried things"), I suggest you squash all of them to avoid conflicts when rebasing. Specifically, that means:
The reason I'm worried about conflicts (that you might not know how to handle) is that you've been doing a lot of back and forth with your commits, doing things in a way that's incompatible with upstream then fixing them later, which will result in a conflict when doing the wrong thing and another one between the resolution of that conflict and correcting to the right thing. A general rule for making rebases painless is to fix your commits instead of leaving the issues and adding other commits on top. One last thing, in interactive rebases, removing a commit from the list actually removes the commit from your branch, so make sure you don't remove something thinking that will "make git ignore that commit" or something. I've seen enough colleagues make that mistake to always point this out when presenting rebase to someone ^^ |
b6bd0bb to
f8c8e9c
Compare
|
Ok, everything has been squashed down to a single commit and all tests are passing :) |
|
Sorry for chiming in late but I think we need to revert and change this :-( From a pure API/CLI standpoint it does not make sense to have an option modify the core behavior of a command: the It would be much more suited as an option or subcommand of the |
|
@sdispater there are a lot of CLIs that do this, some examples: so I think it's a pretty common flag that users would be familiar with |
|
@nicoleclearmetal The examples you give are not really relevant to this case since the Poetry has a sub command system and also has a |
|
@sdispater This option doesn't seem to be available in poetry 1.1.5. As it's kind of important in a production environment, are there any plans to create a new release with this feature? It's security relevant. |
|
Has this feature been reverted before releasing |
No, the changes are still on master
Seems OK to me, the feature is in master, not in the 1.1 branch. Furthermore, patch bump should only contain bug fixes as per semver. We should see this feature in 1.2.0 hopefully 🙂 |
|
Ahh. Sorry for the noise then. :) I thought since it was in master, it was released. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fixes #453
This PR adds a new command,
poetry lock --check, which returns0ifpoetry.lockis consistent withpyproject.tomland1if it is not. This is useful for CI/CD tooling to have a very fast way of checking the lockfile before building without the re-resolution of the dependency tree thatpoetry lockperforms.Pull Request Check List
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!
Note: If your Pull Request introduces a new feature or changes the current behavior, it should be based
on the
developbranch. If it's a bug fix or only a documentation update, it should be based on themasterbranch.If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!