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/07/03 06:04:19 UTC
[GitHub] [airflow] rino0601 opened a new pull request, #24810: Revert "Add %z for %(asctime)s to fix timezone for logs on UI (#24373)"
rino0601 opened a new pull request, #24810:
URL: https://github.com/apache/airflow/pull/24810
follow up of #24373
This reverts commit 7de050ceeb381fb7959b65acd7008e85b430c46f
#24373 modified `.../taskInstance/Logs/utils.js` , so a conflict occurs when cherry-picking to the v2-3-test branch. To solve the problem, revert #24373 and resubmit a new PR. The new pr not only resolves conflicts, it also resolves code duplication issues.
When this pr is merged, subsequent prs must also be merged.
---
**^ 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+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 newsfragment 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] bbovenzi merged pull request #24810: Revert "Add %z for %(asctime)s to fix timezone for logs on UI (#24373)"
Posted by GitBox <gi...@apache.org>.
bbovenzi merged PR #24810:
URL: https://github.com/apache/airflow/pull/24810
--
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] bbovenzi commented on pull request #24810: Revert "Add %z for %(asctime)s to fix timezone for logs on UI (#24373)"
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #24810:
URL: https://github.com/apache/airflow/pull/24810#issuecomment-1176564474
> @rino0601, can't we just merge #24811, which contains the revert and will just effectively update to the proper fix? Not sure keeping a separate revert commit buys us anything?
I agree. One PR would be better.
--
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] rino0601 commented on pull request #24810: Revert "Add %z for %(asctime)s to fix timezone for logs on UI (#24373)"
Posted by GitBox <gi...@apache.org>.
rino0601 commented on PR #24810:
URL: https://github.com/apache/airflow/pull/24810#issuecomment-1177082287
@jedcunningham @bbovenzi
If only #24811 is merged, it will be a squash commit mixed with changes to `airflow/www/static/js/grid/details/content/taskInstance/Logs/utils.js` (here in after `Logs/utils.js` ).
Because `v2-3-stable` branch doesn't have `Logs/utils.js` yet,
changing `Logs/utils.js` will cause a conflict when we `git cherry-pick` on `v2-3-test` later.
To avoid conflict, there are 3 options IMHO
**opt1. cherry-pick a dependency commit before cherry-pick #24811**
the commit creating `Logs/utils.js` needs to be cherry-picked first. `Logs/utils.js` appears since #24404. It has several related PRs and looks still in progress to complete feature. If we only cherry-pick #24404 , it will contain an incomplete function and is not appropriate.
On the other hand, if we cherry-pick all related commits first, it does not seem appropriate for patch release.
**opt2. handle conflict when #24811 is cherry-picked**
maintainer who cherry-picks into `v2-3-test` have to manually fix the conflict.
which is need a lot of efforts.
I think this is the reason why the previous PR(#24373) was omitted in 2.3.3
**opt3. exclude changing `Logs/utils.js`**
So I divided the PR into two. If a revert PR merged to main first, #24811 's sqaush commit don't change `Logs/utils.js` , it makes cherry-picking easier.
There is also a way to make a PR that is easy to cherry-pick without reverting, but then useless regular expressions are left in `Logs/utils.js`. If left unattended, it will become a factor of potential bugs, so we will have to clean it by a separate PR. The only difference is whether we do it first or later to clean the bad codes that already merged, either way, we need two PRs.
If #24403 and it's related can be released as 2.3.4, there is no need to split the PR. (can choose opt1)
However, those PRs are unlabeled, but they are looks very new features... so they're likely going to 2.4.0. I think.
I hope this has been explained, but if you still think that one PR is better, please close this PR. Then, I will write down what to do when a conflict occurs in the #24811. But my English is a mess, so I don't know if it can be explained.
Or if I'm misunderstanding something, please let me know.
--
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] jedcunningham commented on pull request #24810: Revert "Add %z for %(asctime)s to fix timezone for logs on UI (#24373)"
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #24810:
URL: https://github.com/apache/airflow/pull/24810#issuecomment-1176539144
@rino0601, can't we just merge #24811, which contains the revert and will just effectively update to the proper fix? Not sure keeping a separate revert commit buys us anything?
--
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