You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/05/17 18:50:19 UTC
[GitHub] [airflow] potiuk opened a new pull request, #23762: Split contributor's quick start into separate guides.
potiuk opened a new pull request, #23762:
URL: https://github.com/apache/airflow/pull/23762
The foldable parts were not good. They made links not to work as
well as they were not too discoverable.
Fixes: #23174
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragement file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1132027141
I think it's ready for the Summit's workshop ! We have 24 (!) people signed up so we better have it right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1129213111
Refactored quick-start contributor's guide to separate docs before the upcoming summit workshop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
ashb commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1129690072
/cc @RNHTTR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1130599099
As we are preparing for the next week's workshop - do let me know (or maybe even add fixup).
CC: @Bowrna @edithturn @Melodie97 -> would be great to hear your comments/suggestions here too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1130600334
Direct link here: https://github.com/potiuk/airflow/blob/split-contributors-quick-start/CONTRIBUTORS_QUICK_START.rst
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
Bowrna commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1132439887
@potiuk Testing in VS Code follows older breeze commands https://github.com/apache/airflow/blob/main/CONTRIBUTORS_QUICK_START.rst#id11
while testing in PyCharm follows the new breeze commands
https://github.com/apache/airflow/blob/main/CONTRIBUTORS_QUICK_START.rst#testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1131743083
Thanks @edithturn - corrected those. I reviewed the part with Forking and cloning (and referred to Github instructions)- and explained what Breeze is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1130595665
It should be WAY better now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1133667276
Fixed! Thanks @Bowrna @edithturn @RNHTTR . I think we can merge it :).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] edithturn commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
edithturn commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1130754831
@potiuk I was checking:
* I think in this part: "[Forking and cloning Project](https://github.com/potiuk/airflow/blob/split-contributors-quick-start/CONTRIBUTORS_QUICK_START.rst#id7)", cloning is not clear, could be better: on your local machine clone git clone URL. Because I think [Setting up Breeze](https://github.com/potiuk/airflow/blob/split-contributors-quick-start/CONTRIBUTORS_QUICK_START.rst#id8) is after cloning the repo.
* After cloning the repo it goes for Breeze, and I think we need to add something to say, "what is a breeze and why we need it, or a link for Breeze" (I saw the reference at the end)
* Mention pre-commits are great! 💃🏼
It is shorted now, and better @potiuk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RNHTTR commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
RNHTTR commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1130521006
A few thoughts:
1. I think it'd make sense to mention Breeze in [CONTRIBUTORS_QUICK_START.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START.rst). Maybe just a high level overview and a link to [BREEZE.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/BREEZE.rst). Perhaps in the "Note to Starters" or "Local machine development" sections?
2. My concern would be how maintainable will these guides be? If something common to each of the guides changes, they're not very DRY, and the update will have to be made in each sub-guide. Maybe common parts like "Forking and cloning Project", "Setting up Breeze", "Installing airflow in the local virtual environment airflow-env with breeze.", "Using Breeze", "Testing", etc could be included in the "main" [CONTRIBUTORS_QUICK_START.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START.rst). Perhaps there could be a link at the end of each sub-guide that returns to `CONTRIBUTORS_QUICK_START.rst` to finish the guide with these sections.
3. Nitpick, but in [CONTRIBUTORS_QUICK_START.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START.rst), maybe it should say "Contributor's Quick Start" or "Contributor's Quick Start Guide" instead of "Contributor's Quick Guide".
4. Another nitpick: IMO [CONTRIBUTORS_QUICK_START_VSC.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START_VSC.rst) should be named `CONTRIBUTORS_QUICK_START_VSCODE.rst`.
5. Yet another nitpick: Each of the sub-guides starts with "Setup Airflow with Breeze" and "Forking and cloning Project" as a header. Maybe these headers should be removed; I'm not sure there should be a header for Breeze here because it's explained later in the guides, and there's no alternative to Breeze given. Does that make sense? Similarly, maybe the "Forking and cloning Project" header should be removed because it's described in step 1 and there's no alternative.
Overall I think this is much better! Let me know what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1130592634
> A few thoughts:
>
> 1. I think it'd make sense to mention Breeze in [CONTRIBUTORS_QUICK_START.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START.rst). Maybe just a high level overview and a link to [BREEZE.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/BREEZE.rst). Perhaps in the "Note to Starters" or "Local machine development" sections?
Good point. Done.
> 2. My concern would be how maintainable will these guides be? If something common to each of the guides changes, they're not very DRY, and the update will have to be made in each sub-guide. Maybe common parts like "Forking and cloning Project", "Setting up Breeze", "Installing airflow in the local virtual environment airflow-env with breeze.", "Using Breeze", "Testing", etc could be included in the "main" [CONTRIBUTORS_QUICK_START.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START.rst). Perhaps there could be a link at the end of each sub-guide that returns to `CONTRIBUTORS_QUICK_START.rst` to finish the guide with these sections.
Yeah. I think there was a bit too much duplication now when I looked at it. Should be much better now - I actually left jus the vscode/pycharm/gitpod/codespaces specific parts. This makes a bit more "go here", and then "click here", but with the amount of duplication we had it made indeed little sense.
> 3. Nitpick, but in [CONTRIBUTORS_QUICK_START.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START.rst), maybe it should say "Contributor's Quick Start" or "Contributor's Quick Start Guide" instead of "Contributor's Quick Guide".
Ah yeah - did not see that :)
> 4. Another nitpick: IMO [CONTRIBUTORS_QUICK_START_VSC.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START_VSC.rst) should be named `CONTRIBUTORS_QUICK_START_VSCODE.rst`.
surely
> 5. Yet another nitpick: Each of the sub-guides starts with "Setup Airflow with Breeze" and "Forking and cloning Project" as a header. Maybe these headers should be removed; I'm not sure there should be a header for Breeze here because it's explained later in the guides, and there's no alternative to Breeze given. Does that make sense? Similarly, maybe the "Forking and cloning Project" header should be removed because it's described in step 1 and there's no alternative.
>
I reworked it quite a bit now in general. I think it will be much nicer.
> Overall I think this is much better! Let me know what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1133832949
Any commmiter review - most welcome here :).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1136096836
🙏 - I want to send links to participants of the workshop @eladkal :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1133666816
> Testing in VS Code and GitPod follows older breeze commands
Ah goood one. We already removed the duplication in the "split" version - this was one of the artifacts of the "doc duplication" that we had. Now there is only one section about it .... but it uses the legacy way 🤦
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23762:
URL: https://github.com/apache/airflow/pull/23762#issuecomment-1136126700
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #23762: Split contributor's quick start into separate guides.
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #23762:
URL: https://github.com/apache/airflow/pull/23762
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org