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

Incorrect docstring for apply_wildcard parameter in S3ListOperator #36593

Closed
1 of 2 tasks
simond opened this issue Jan 4, 2024 · 6 comments
Closed
1 of 2 tasks

Incorrect docstring for apply_wildcard parameter in S3ListOperator #36593

simond opened this issue Jan 4, 2024 · 6 comments

Comments

@simond
Copy link
Contributor

simond commented Jan 4, 2024

What do you see as an issue?

The docstring for the S3ListOperator is incorrect for the apply_wildcard parameter:

:param apply_wildcard: whether to treat '*' as a wildcard or a plain symbol in the prefix.
        By default SSL certificates are verified.
        You can provide the following values:


        - ``False``: do not validate SSL certificates. SSL will still be used
                 (unless use_ssl is False), but SSL certificates will not be
                 verified.
        - ``path/to/cert/bundle.pem``: A filename of the CA cert bundle to uses.
                 You can specify this argument if you want to use a different
                 CA cert bundle than the one used by botocore.

Solving the problem

It looks as though most of the docstring should actually be part of the verify and not the apply_wildcard one. Moving the following lines up under the verify parameter should resolve:

        By default SSL certificates are verified.
        You can provide the following values:


        - ``False``: do not validate SSL certificates. SSL will still be used
                 (unless use_ssl is False), but SSL certificates will not be
                 verified.
        - ``path/to/cert/bundle.pem``: A filename of the CA cert bundle to uses.
                 You can specify this argument if you want to use a different
                 CA cert bundle than the one used by botocore.

Anything else

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@simond simond added kind:bug This is a clearly a bug kind:documentation needs-triage label for new issues that we didn't triage yet labels Jan 4, 2024
Copy link

boring-cyborg bot commented Jan 4, 2024

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

@Taragolis Taragolis added good first issue and removed kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Jan 4, 2024
@potiuk
Copy link
Member

potiuk commented Jan 4, 2024

Feel free to open PR for it @simond . It seems that creating PR for one would take less time than describing it in an issue.

@simond
Copy link
Contributor Author

simond commented Jan 4, 2024

I would normally agree but, last time I checked, setting up the development environment for Airflow to do a PR with tests etc was quite a lot of work.

@potiuk
Copy link
Member

potiuk commented Jan 4, 2024

I would normally agree but, last time I checked, setting up the development environment for Airflow to do a PR with tests etc was quite a lot of work.

Well. It's ~ 15 minutes - mosty to pull images - to run Breeze (after installing Docker + Docker Compose + pipx basically) so I think it's not as difficult as you might think. And yes. Improvements to installing local environment (including using Hatch which manages Python installations and virtualenvs is coming sooon - See #36537 - so you might want to re-check it back when it's merged if you do not want to use Docker.

Maybe comment in the PR to get notified when it gets merged so that you can try it then ?

@simond
Copy link
Contributor Author

simond commented Jan 4, 2024

I would normally agree but, last time I checked, setting up the development environment for Airflow to do a PR with tests etc was quite a lot of work.

Well. It's ~ 15 minutes - mosty to pull images - to run Breeze (after installing Docker + Docker Compose + pipx basically) so I think it's not as difficult as you might think. And yes. Improvements to installing local environment (including using Hatch which manages Python installations and virtualenvs is coming sooon - See #36537 - so you might want to re-check it back when it's merged if you do not want to use Docker.

Maybe comment in the PR to get notified when it gets merged so that you can try it then ?

I'll take another look as it sounds like it has changed since I looked last, thanks!

@josh-fell
Copy link
Contributor

Closed via #36679.

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

No branches or pull requests

4 participants