[sblondon]

Pull Request Check List

Resolves: #3567

• Added tests for changed code.
• Updated documentation for changed code.

This PR adds a logging.warning() if some credentials are written in the lock file. It's based on this comment.

I wonder if changing the attribute (Locker.contains_credential) to a method would be better (or not). The message could probably be improved but I did not find the better sentence.

[Secrus]

Hi, @sblondon are you still interested in bringing this to Poetry? If so, please sync changes from the master branch and resolve conflicts. That would allow for review.

[sblondon]

@Secrus I still think it would be better to add the warning. I rebased the patch (and resolve the conflicts).

All environments fail in CI failed because there is an error when poetry run mypymypy' in src/poetry/utils/env.py. I don't think it's linked to the patch. I will search if it exists on master too.

[neersighted]

Two minor changes -- sorry for forgetting to ask for the property to be renamed earlier.

Additionally, we need to get CI passing so we can merge this.

[sblondon]

Additionally, we need to get CI passing so we can merge this.

Yes, I have to understand why it fails since the rebase.

[dimbleby]

This looks like a frustrating error message - I might plausibly have hundreds of packages in poetry.lock, being told that one of them has credentials is going to leave me with work to do...

Can this warning say which package is the problematic one?

[sblondon]

The warning is not triggered by a package. It is displayed if credentials are in the URL of an added source in package.source section.

[dimbleby]

Those credentials are associated with a particular package, no? eg in your testcase, it's package A that triggers the warning.

If I have a lockfile with hundreds of packages in it, then a more helpful warning would point me at A rather than only telling me that there's a problem, somewhere.

[sblondon]

When I got this issue, the URL coming from the package.source was added to every packages in the lock file. So it's not useful. However, adding the URL in the message could help the user:

Package URL contain credentials (https://alice:a_passwd@pypi.domain.tld/site/prod/+simple) and will be written to poetry.lock.

From file:

[package.source]
type = "legacy"
url = "https://alice:a_passwd@pypi.domain.tld/site/prod/+simple"
reference = "whatever"

[neersighted]

This LGTM, except I think we should resolve this thread. Can we include the URL?

[sblondon]

@neersighted I can modify _contains_credential to be the URL instead of a boolean status. In such case, the name fits less well. If it's still ok fo you, I'll be happy to modify it. An other way would be to rename it but it will also require to rename it in the poetry-plugin-export repository. The third way is to not add the URL. My favorite way is the first one (modifying the content of _contains_credential). What do you prefer?

[sblondon]

@neersighted The error in CI is not the tests of poetry ('Run pytest' and 'Run test (integration suite)' are green) but the tests in
poetry-plugin-export step fail ('Run pytest (poetry-plugin-export)').

tests.helpers.TestLocker class exists in python-poetry and poetry-plugin-export repositories. In poetry-plugin-export repository, tests.helpers.TestLocker class does not mock _contains_credential attribute . _contains_credential attribute is added in this PR so it has never been necessary in TestLocker mock in poetry-plugin-export. So the tests raise AttributeError: 'TestLocker' object has no attribute '_contains_credential'.

To fix the error, I can do a PR to add the attribute in the tests.helpers.TestLocker.__init__ method in poetry-plugin-export repository.

What do you think about it?

[neersighted]

@neersighted The error in CI is not the tests of poetry ('Run pytest' and 'Run test (integration suite)' are green) but the tests in poetry-plugin-export step fail ('Run pytest (poetry-plugin-export)').

tests.helpers.TestLocker class exists in python-poetry and poetry-plugin-export repositories. In poetry-plugin-export repository, tests.helpers.TestLocker class does not mock _contains_credential attribute . _contains_credential attribute is added in this PR so it has never been necessary in TestLocker mock in poetry-plugin-export. So the tests raise AttributeError: 'TestLocker' object has no attribute '_contains_credential'.

To fix the error, I can do a PR to add the attribute in the tests.helpers.TestLocker.__init__ method in poetry-plugin-export repository.

What do you think about it?

Sorry I never saw this/got back to you, @sblondon... Feel free to make a PR against that repo and link to it here. However, I suspect that we'll simply try and decouple the testing of the two repos in the near future ala #5980.

[sblondon]

I created a PR in python-poetry/poetry-plugin-export#93 to add the TestLocker._contains_credential attribute in the library.

I agree it would be good to fix such dependency between the two repositories. The changes in TestLocker.__init__ classes from poetry and poetry-plugin-export are identical so the refactoring will not be harder.

[sblondon]

The patch in poetry-plugin-extra is merged.
When a new release will be available, I will update the dependency in pyproject.toml in order to fix the tests.

[Secrus]

Stalled

[github-actions]

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.