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

Apply auth also for private pypi packages #323

Merged
merged 9 commits into from
Feb 16, 2023
Merged

Conversation

croth1
Copy link
Contributor

@croth1 croth1 commented Jan 29, 2023

previous logic converted all lock files to explicit type and ignored all lines starting with #or @. This however does not allow adding auth to packages from private pypi indices, as they are represented as # pip package @ index_url and thus were skipped in the auth re-adding logic.

@netlify
Copy link

netlify bot commented Jan 29, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 801110f
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/63d6d483ab5f5c0009afac5c
😎 Deploy Preview https://deploy-preview-323--conda-lock.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@croth1
Copy link
Contributor Author

croth1 commented Feb 7, 2023

@maresb whom do I need to bribe to get this reviewed? :)

@maresb
Copy link
Contributor

maresb commented Feb 8, 2023

Hi @croth1! I don't have much time these days, but at least this one is nice and short!

You could start by bribing me with a test case so that we can prevent regressions. 🙂 Thanks!

@croth1 croth1 requested a review from a team as a code owner February 8, 2023 20:15
@croth1
Copy link
Contributor Author

croth1 commented Feb 14, 2023

@maresb, I integrated the new logic into existing test cases - strictly speaking the auth stripping logic does not seem to be needed for pypi, as the current poetry code does that automatically.
However this way the pypi auth logic is the same as the conda auth logic and was a bit easier to integrate into the tests. Hope that's fine with you.

Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

Thanks so much @croth1 for your persistence and patience on this!!!

(I wasn't so familiar with this code, so even though this is straightforward, it's been tricky for me to find the time to look properly at this. Not to mention that the current directory structure for these test files confuses me; perhaps we should eventually clean that up.)

This looks really excellent. Could you please add test cases for test__add_auth_to_line, test__strip_auth_from_line, test__extract_domain? Luckily these tests are self-contained, so I think this will be quick and easy.

Once that's done, it looks good from my side. Anything to add, @mariusvniekerk?

conda_lock/conda_lock.py Outdated Show resolved Hide resolved
conda_lock/conda_lock.py Outdated Show resolved Hide resolved
Co-authored-by: Ben Mares <services-git-throwaway1@tensorial.com>
@netlify
Copy link

netlify bot commented Feb 16, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 0e7bd67
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/63ee998ca352a1000868de96
😎 Deploy Preview https://deploy-preview-323--conda-lock.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@maresb
Copy link
Contributor

maresb commented Feb 16, 2023

Thanks so much @croth1!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants