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

Reorganise issue template for providers wrt to testing process link #33439

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

pankajkoti
Copy link
Member

Currently, the header of the issue template requests to test the RCs.
However, the guideline stating how to test those providers is placed at
the bottom of the issue (appearing a bit disconnected) and might not
be visible easily when have to scroll to the bottom end of the list
of the Provider RCs. I think it is more evident and helpful if we advertise
it in the beginning.
e.g. we can see here #33305 how it appears currently.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Currently, the header of the issue template requests to test the
RCs. However the guideline stating how to test those providers
is placed at the bottom of the issue (appearing a bit disconnected)
and might not be visible easily when have to scroll to the bottom end
through the list of the Provider RCs.
I think it is more evident and helpful if we advertise it at the
begining.
e.g. we can see here apache#33305 on how it appears currently.
@@ -1,6 +1,10 @@
I have a kind request for all the contributors to the latest provider packages release.
Could you please help us to test the RC versions of the providers?

The guidelines on how to test providers can be found in

[Verify providers by contributors](https://github.com/apache/airflow/blob/main/dev/README_RELEASE_PROVIDER_PACKAGES.md#verify-the-release-candidate-by-contributors)
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW the doc says:
NOTE! You should Ctrl-C and restart the connections to restart airflow components and make sure new provider packages is used.

I think we should further clarify also on how to do it rather than say what needs to be done.
(explain what command to run to restart and explain how to do the verify)

Copy link
Member

@potiuk potiuk Aug 16, 2023

Choose a reason for hiding this comment

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

@eladkal -> What would you suggest ? This is unrelated and should be separate PR but I agree 👍

NOTE! You should Ctrl-C and restart the connections to restart airflow components and make sure new provider packages is used.

is quite wrong .

The idea is that you need to restart the components to get the new providers loaded by new interpreters. Maybe you can phrase it well?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, can do it in a separate PR :)

Do we mean to elaborate on the steps, like in each of the pane in Tmux session, kill the current process and restart using the corresponding commands like airflow webserver, airflow scheduler, airflow triggerer, airflow celery worker (if using CeleryExecutor), etc.?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah totally separate PR.

yes, we should have explnation of what we want to do, why we are doing it and how to actually do it.

I think ideally breeze should handle it by it's own (thus making the paragraph we want to introduce now redundant)
Something like:

breeze start-airflow --use-airflow-version 2.2.4 --python 3.8 --backend postgres \
    --load-example-dags --load-default-connections --override-default-provider-versions apache-airflow-providers-amazon==10.10.0rc1

but that is a feature request for later.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, Breeze can already handle it when specifying it at startup. We have the section Manually testing release candidate packages linked in the same document

and @potiuk recently enhanced that section a lot by giving numerous examples in PR #32948.

Copy link
Member

@potiuk potiuk Aug 17, 2023

Choose a reason for hiding this comment

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

Yeah totally separate PR.

yes, we should have explnation of what we want to do, why we are doing it and how to actually do it.

I think ideally breeze should handle it by it's own (thus making the paragraph we want to introduce now redundant) Something like:

breeze start-airflow --use-airflow-version 2.2.4 --python 3.8 --backend postgres \
    --load-example-dags --load-default-connections --override-default-provider-versions apache-airflow-providers-amazon==10.10.0rc1

but that is a feature request for later.

Indeed - this precisely what I improved for Airflow recently (and I think it has all the aspects of what you are talking about @eladkal). It would be great if someone takes it as an example and distills that into better instructions if the current provider's ones are not good enough. Other than correcting the strange sentence I am not sure how to enhance current instructions, though?

Maybe someone could create a PR with better description and we could iterate on it to make it clearer ? I am not sure how to more describe the whys and whether the current provider instructions fail to deliver on that @eladkal ?

For the reference this is the current description for Airflow.

Verify the release candidate by Contributors
This can be done (and we encourage to) by any of the Contributors. In fact, it's best if the actual users of Apache Airflow test it in their own staging/test installations. Each release candidate is available on PyPI apart from SVN packages, so everyone should be able to install the release candidate version.

But you can use any of the installation methods you prefer (you can even install it via the binary wheels downloaded from the SVN).

Installing release candidate in your local virtual environment
pip install apache-airflow==rc
Optionally it can be followed with constraints

pip install apache-airflow==rc
--constraint "https://raw.githubusercontent.com/apache/airflow/constraints-/constraints-3.8.txt"`
Note that the constraints contain python version that you are installing it with.

You can use any of the installation methods you prefer (you can even install it via the binary wheel downloaded from the SVN).

There is also an easy way of installation with Breeze if you have the latest sources of Apache Airflow. Running the following command will use tmux inside breeze, create admin user and run Webserver & Scheduler:

breeze start-airflow --use-airflow-version 2.7.0rc1 --python 3.8 --backend postgres
You can also choose different executors and extras to install when you are installing airflow this way. For example in > order to run Airflow with CeleryExecutor and install celery, google and amazon provider (as of Airflow 2.7.0, you need > to have celery provider installed to run Airflow with CeleryExecutor) you can run:

breeze start-airflow --use-airflow-version 2.7.0rc1 --python 3.8 --backend postgres
--executor CeleryExecutor --airflow-extras "celery,google,amazon"
Once you install and run Airflow, you should perform any verification you see as necessary to check that the Airflow works as you expected.



@pankajkoti pankajkoti marked this pull request as ready for review August 16, 2023 13:42
@potiuk potiuk merged commit 8b7e0ba into apache:main Aug 17, 2023
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants