You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "amoghrajesh (via GitHub)" <gi...@apache.org> on 2023/07/17 11:47:46 UTC
[GitHub] [airflow] amoghrajesh opened a new pull request, #32649: Fixing breeze add-back-references command with doc
amoghrajesh opened a new pull request, #32649:
URL: https://github.com/apache/airflow/pull/32649
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
A failure was reported in using this command. This PR intends to fix it.
Reference: https://github.com/apache/airflow/pull/32613#discussion_r1264883314
<!--
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 an 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 changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 newsfragment file, named `{pr_number}.significant.rst` or `{issue_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] amoghrajesh commented on a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1265789580
##########
dev/breeze/src/airflow_breeze/commands/release_management_commands.py:
##########
@@ -821,7 +821,6 @@ def publish_docs(
"--airflow-site-directory",
envvar="AIRFLOW_SITE_DIRECTORY",
help="Local directory path of cloned airflow-site repo.",
- required=True,
Review Comment:
Yeah seems to work fine. Bringing it back
--
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] amoghrajesh commented on pull request #32649: Fixing breeze add-back-references command with doc
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32649:
URL: https://github.com/apache/airflow/pull/32649#issuecomment-1638231498
> --airflow-site-directory
Hi @potiuk, just to understand better, should I not make any command in release management accept the `airflow-site-dierectory` parameter at all?
--
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] amoghrajesh commented on a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1265760108
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -392,8 +393,7 @@ breeze release-management publish-docs \
--package-filter 'apache-airflow-providers-*' \
--override-versioned
-cd "${AIRFLOW_SITE_DIRECTORY}"
Review Comment:
Thanks for that info, @potiuk
I will try it out now and in the future PRs 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 merged pull request #32649: Fixing breeze add-back-references command with doc
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #32649:
URL: https://github.com/apache/airflow/pull/32649
--
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] eladkal commented on a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1278522426
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -392,10 +392,7 @@ breeze release-management publish-docs \
--package-filter 'apache-airflow-providers-*' \
--override-versioned
-cd "${AIRFLOW_SITE_DIRECTORY}"
-breeze release-management add-back-references --airflow-site-directory --gen-type providers
-cd ..
-
+breeze release-management add-back-references --gen-type providers
Review Comment:
@aminghadersohi I suspect this doesn't work as expected for redirects between providers like was done in https://github.com/apache/airflow/pull/32306
This caused creation of latest folder in the Google provider:
<img width="296" alt="Screenshot 2023-07-30 at 10 13 15" src="https://github.com/apache/airflow/assets/45845474/55bed2b0-a532-4875-971e-3a669c585ff4">
Which caused failure in doc site build https://github.com/apache/airflow-site/actions/runs/5704523295/job/15458213062
When I removed the latest folder build ran successfully
https://github.com/apache/airflow-site/pull/840/commits/f8b14602eb9c793926fdfd280b876ea6bbd86dec
--
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] amoghrajesh commented on a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1265445748
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -381,6 +381,7 @@ If you have providers as list of provider ids because you just released them, yo
```
- Copy the documentation to the ``airflow-site`` repository
+- Either provide the ``--airflow-site-directory`` flag or set the env variable: ``AIRFLOW_SITE_DIRECTORY``
Review Comment:
Yeah, doesn't make sense anymore. There were some earlier discussions leading to this decision
--
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] eladkal commented on a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1278543146
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -392,10 +392,7 @@ breeze release-management publish-docs \
--package-filter 'apache-airflow-providers-*' \
--override-versioned
-cd "${AIRFLOW_SITE_DIRECTORY}"
-breeze release-management add-back-references --airflow-site-directory --gen-type providers
-cd ..
-
+breeze release-management add-back-references --gen-type providers
Review Comment:
Ah NVM this is issue in the path https://github.com/apache/airflow/pull/32945
I guess we can work on a pre commit to avoid such cases in the future
--
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] amoghrajesh commented on a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1265444961
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -392,8 +393,7 @@ breeze release-management publish-docs \
--package-filter 'apache-airflow-providers-*' \
--override-versioned
-cd "${AIRFLOW_SITE_DIRECTORY}"
Review Comment:
Noted. Will keep in mind while updating the PE
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -392,8 +393,7 @@ breeze release-management publish-docs \
--package-filter 'apache-airflow-providers-*' \
--override-versioned
-cd "${AIRFLOW_SITE_DIRECTORY}"
Review Comment:
Noted. Will keep in mind while updating the PR
--
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 #32649: Fixing breeze add-back-references command with doc
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32649:
URL: https://github.com/apache/airflow/pull/32649#issuecomment-1638217318
Somehow it has not been fixed in the previous `--airflow-site-directory DIRECTORY` -> this is what I referred to before - just remove it altoghether from all commands (including `--airflow-site-directory`). The AIRFLOW_SITE_DIRECTORY is set in the previous step, so you should not need to add it here.
Those are copy-pasteable instructions, you should not add `DIRECTORY` here because that would make the one who follows the instruction having to replace it with the real directory manually and we do NOT want it. We want release manager to be able to just copy*paste those instructions (this is what AIRFLOW_SITE_DIRECTORY env var settings in the previous step is supposed to achieve).
--
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 a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1265773973
##########
dev/breeze/src/airflow_breeze/commands/release_management_commands.py:
##########
@@ -821,7 +821,6 @@ def publish_docs(
"--airflow-site-directory",
envvar="AIRFLOW_SITE_DIRECTORY",
help="Local directory path of cloned airflow-site repo.",
- required=True,
Review Comment:
It's ok to keep it required I believe + the env var is also providing required value
--
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 a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1265419303
##########
dev/README_RELEASE_AIRFLOW.md:
##########
@@ -802,10 +802,11 @@ Documentation for providers can be found in the ``/docs/apache-airflow`` directo
```
- Copy the documentation to the ``airflow-site`` repository, create commit, push changes, open a PR and merge it when the build is green.
+- Either provide the ``--airflow-site-directory`` flag or set the env variable: ``AIRFLOW_SITE_DIRECTORY``
Review Comment:
This is not needed. The `AIRFLOW_SITE_DIRECTORY` is already set in line 788. so this is not something optional - we already KNOW it has been set.
Same in all other cases.
--
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] amoghrajesh commented on pull request #32649: Fixing breeze add-back-references command with doc
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32649:
URL: https://github.com/apache/airflow/pull/32649#issuecomment-1638231495
> --airflow-site-directory
Hi @potiuk, just to understand better, should I not make any command in release management accept the `airflow-site-dierectory` parameter at all?
--
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] amoghrajesh commented on pull request #32649: Fixing breeze add-back-references command with doc
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32649:
URL: https://github.com/apache/airflow/pull/32649#issuecomment-1638684711
@potiuk @eladkal tested all the scenarios. They seem to work well with the current PR state 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] eladkal commented on a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1265426635
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -392,8 +393,7 @@ breeze release-management publish-docs \
--package-filter 'apache-airflow-providers-*' \
--override-versioned
-cd "${AIRFLOW_SITE_DIRECTORY}"
Review Comment:
If you remove this cd then please remove also the `cd ..` in line 397
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -381,6 +381,7 @@ If you have providers as list of provider ids because you just released them, yo
```
- Copy the documentation to the ``airflow-site`` repository
+- Either provide the ``--airflow-site-directory`` flag or set the env variable: ``AIRFLOW_SITE_DIRECTORY``
Review Comment:
AIRFLOW_SITE_DIRECTORY is always point to site directory it's part of release process. I'm not sure what is the value of this line?
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -392,8 +393,7 @@ breeze release-management publish-docs \
--package-filter 'apache-airflow-providers-*' \
--override-versioned
-cd "${AIRFLOW_SITE_DIRECTORY}"
-breeze release-management add-back-references --airflow-site-directory --gen-type providers
+breeze release-management add-back-references --airflow-site-directory DIRECTORY --gen-type providers
Review Comment:
Do you mean to run this command we need to manually replace the DIRECTORY key word?
--
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] amoghrajesh commented on a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1265788557
##########
dev/breeze/src/airflow_breeze/commands/release_management_commands.py:
##########
@@ -821,7 +821,6 @@ def publish_docs(
"--airflow-site-directory",
envvar="AIRFLOW_SITE_DIRECTORY",
help="Local directory path of cloned airflow-site repo.",
- required=True,
Review Comment:
Oh ok, let me try it out with that factor.
--
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] amoghrajesh commented on pull request #32649: Fixing breeze add-back-references command with doc
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #32649:
URL: https://github.com/apache/airflow/pull/32649#issuecomment-1638074922
@eladkal this should fix the issue you ran into earlier today. Can you help with a review when you have some time?
--
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] amoghrajesh commented on a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1265446460
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -392,8 +393,7 @@ breeze release-management publish-docs \
--package-filter 'apache-airflow-providers-*' \
--override-versioned
-cd "${AIRFLOW_SITE_DIRECTORY}"
-breeze release-management add-back-references --airflow-site-directory --gen-type providers
+breeze release-management add-back-references --airflow-site-directory DIRECTORY --gen-type providers
Review Comment:
Yeah that was the initial idea. But that makes the release management team's life inconvenient. Improvising 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 a diff in pull request #32649: Fixing breeze add-back-references command with doc
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32649:
URL: https://github.com/apache/airflow/pull/32649#discussion_r1265687711
##########
dev/README_RELEASE_PROVIDER_PACKAGES.md:
##########
@@ -392,8 +393,7 @@ breeze release-management publish-docs \
--package-filter 'apache-airflow-providers-*' \
--override-versioned
-cd "${AIRFLOW_SITE_DIRECTORY}"
Review Comment:
BTW. I think the best thing is to just ... execute the processs... this way is the only way to check if it works @amoghrajesh - and it does not have to be complete. Like pushing the code does not neede to happen, and up to pushing to airflow repo, all steps can be done by you as well.
--
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