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

Update upgrading.rst with detailed code example of how to resolve post-upgrade warning #19993

Merged
merged 6 commits into from
Dec 19, 2021

Conversation

adaezebestow
Copy link
Contributor

Give a detailed code example for how to drop the _airflow_moved__2_2__task_instance table to resolve post-upgrade warning


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

Give a detailed code example for how to drop the _airflow_moved__2_2__task_instance table to resolve post-upgrade warning
@boring-cyborg
Copy link

boring-cyborg bot commented Dec 2, 2021

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://s.apache.org/airflow-slack

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Dec 2, 2021
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member

potiuk commented Dec 2, 2021

Nice! I really like when we give "good" guidance to our users.

@potiuk
Copy link
Member

potiuk commented Dec 3, 2021

Some whitespace problem :(

@potiuk
Copy link
Member

potiuk commented Dec 5, 2021

I think there is an empty line problem

Alternatively, you can drop the table by following the steps below:


1. Exec into any of the Airflow pods - webserver or scheduler: ``kubectl exec -it <your-webserver-pod> python``
Copy link
Member

Choose a reason for hiding this comment

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

The command example here is out of place since it is specific to Kubernetes, while the rest of the document does not make the assumption, and already assumes you have hooked into the machine/container/pod/whatever. I would only add the second bullet point.

Copy link
Member

@potiuk potiuk Dec 6, 2021

Choose a reason for hiding this comment

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

I'd rather give some more examples - kubernetes, docker, docker-compose and tell the users that this is an example and that they should find out what they should do in their environment.

I think our assumption is that this documentation is used by the users who had never done low-level DB operations with Airflow DB, and they likely have no idea how to get to their containers. Giving them at least a handy example they could follow and at least the starting point that will allow them to ask the right google question to dig deeper is useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is just need to be in a note block that mention this is K8s specific

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The note block makes sense

Noting the steps are specific to Kubernetes
@potiuk
Copy link
Member

potiuk commented Dec 13, 2021

Can we get update on that one :) @adaezebestow ?

@adaezebestow
Copy link
Contributor Author

Hey @potiuk, I made an update specifying that the code suggestion is specific to Kubernetes. I tried unsuccessfully to reformat it into a note block.

@potiuk
Copy link
Member

potiuk commented Dec 13, 2021

Docs are failing - I think you need ot look at similar .rst files :)

something like .. note:: should do the trick

@eladkal eladkal added this to the Airflow 2.2.4 milestone Dec 19, 2021
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

probably it's just missing a blank line.
I submitted a fix it should make the CI happy

@potiuk
Copy link
Member

potiuk commented Dec 19, 2021

Ci Indeed looks happy :)

@potiuk potiuk merged commit 4ac35d7 into apache:main Dec 19, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Dec 19, 2021

Awesome work, congrats on your first merged pull request!

@jedcunningham jedcunningham added the type:doc-only Changelog: Doc Only label Dec 21, 2021
jedcunningham pushed a commit that referenced this pull request Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:documentation okay to merge It's ok to merge this PR as it does not require more tests type:doc-only Changelog: Doc Only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants