airbyte-ci: Test pypi published properly #34689
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
flash1293
changed the title
| python_registry_check_url: | ||
| description: "Python registry URL to check whether a python package is published already" | ||
| default: "https://pypi.org/pypi" | ||
| required: false |
There was a problem hiding this comment.
I'm not sure this input is required as in CI use cases there's no reason to change this URL at the moment.
Same for PYTHON_REGISTRY_URL. The defaults are set at the airbyte-ci CLI level and we have to no reason to change these in the CI ATM. Am I correct?
There was a problem hiding this comment.
that's right, we can also remove them from here.
| python_registry_url: Optional[str] = None, | ||
| python_registry_check_url: Optional[str] = None, |
There was a problem hiding this comment.
I might repeat the same question I asked yesterday, and sorry for that :) :
I'm not sure python_registry_url and python_registry_check_url should be nullable as we're always building contexts with them set to hardcoded defaults.
The is_package_published function expects these attributes to be set.
If I'm understanding the code path correctly the thing which guarantees these are set when executing is_package_published is this if statement. Should we check that the check url is not null there too?
There was a problem hiding this comment.
you are right, as they have a default there is no good reason to make them nullable, I will remove that.
Regular pypi uses different hosts for checking whether a python package exists and for publishing a new one.
This PR accounts for that by adding a new option/env var for the registry check URL and passes it through in a similar fashion to the api token.