-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Fixing breeze add-back-references command with doc #32649
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
Conversation
|
@eladkal this should fix the issue you ran into earlier today. Can you help with a review when you have some time? |
|
Somehow it has not been fixed in the previous Those are copy-pasteable instructions, you should not add |
dev/README_RELEASE_AIRFLOW.md
Outdated
| ``` | ||
|
|
||
| - 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`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| --package-filter 'apache-airflow-providers-*' \ | ||
| --override-versioned | ||
|
|
||
| cd "${AIRFLOW_SITE_DIRECTORY}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you remove this cd then please remove also the cd .. in line 397
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted. Will keep in mind while updating the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that info, @potiuk
I will try it out now and in the future PRs too!
| ``` | ||
|
|
||
| - Copy the documentation to the ``airflow-site`` repository | ||
| - Either provide the ``--airflow-site-directory`` flag or set the env variable: ``AIRFLOW_SITE_DIRECTORY`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, doesn't make sense anymore. There were some earlier discussions leading to this decision
|
|
||
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to run this command we need to manually replace the DIRECTORY key word?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that was the initial idea. But that makes the release management team's life inconvenient. Improvising here
| "--airflow-site-directory", | ||
| envvar="AIRFLOW_SITE_DIRECTORY", | ||
| help="Local directory path of cloned airflow-site repo.", | ||
| required=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok to keep it required I believe + the env var is also providing required value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, let me try it out with that factor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah seems to work fine. Bringing it back
| breeze release-management add-back-references --airflow-site-directory --gen-type providers | ||
| cd .. | ||
|
|
||
| breeze release-management add-back-references --gen-type providers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aminghadersohi I suspect this doesn't work as expected for redirects between providers like was done in #32306
This caused creation of latest folder in the Google provider:
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
apache/airflow-site@f8b1460
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah NVM this is issue in the path #32945
I guess we can work on a pre commit to avoid such cases in the future
A failure was reported in using this command. This PR intends to fix it.
Reference: #32613 (comment)
^ 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.rstor{issue_number}.significant.rst, in newsfragments.