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

Revert "Add %z for %(asctime)s to fix timezone for logs on UI (#24373)" #24810

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

rino0601
Copy link
Contributor

@rino0601 rino0601 commented Jul 3, 2022

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 as #24811.
#24811 not only resolves conflicts, it also resolves code duplication.

When this pr is merged, subsequent PR (#24811) must also be merged.


^ 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.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:logging area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jul 3, 2022
@rino0601 rino0601 marked this pull request as ready for review July 3, 2022 07:34
@potiuk potiuk requested a review from uranusjr July 6, 2022 13:43
@jedcunningham
Copy link
Member

@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?

@bbovenzi
Copy link
Contributor

bbovenzi commented Jul 6, 2022

@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.

@rino0601
Copy link
Contributor Author

rino0601 commented Jul 7, 2022

@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.

@bbovenzi bbovenzi merged commit 8200723 into apache:main Jul 8, 2022
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logging area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants