-
Notifications
You must be signed in to change notification settings - Fork 16.3k
[v3-1-test] Expand and collapse group component (#56293) #56334
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
* Extract component for expand and collapse button group * Refactor toggle groups buttons (cherry picked from commit 6d1991d) Co-authored-by: Pierre Jeambrun <pierrejbrun@gmail.com>
|
Still the same unrelated CI error, merging |
Proposal (i feel like a ghost writing it still from vacations). I think (and I might want to send a message to devlist after I am back from vacations - or rather after we come back from the summit) - that i is it's a bad idea to just "merge" PRs to
I belive we discussed it long time ago and this was more or less what we agreed. For now cc:-ing those who had some relations with backporting in the last few days: @bbovenzi @eladkal @kaxil @gopidesupavan @amoghrajesh @ashb In this case likely rebase would have helped (at least with image building issue that were there for the last few days). I fixed failing CI build few hours ago (from vacations) #56352 (comment) explaining the reason - with comment that it's likely not a good idea to merge PRs to v3-1-test before you solve the underlying issue. This is possibly making release manager job harder. I think some attempt should be made when we merge such PRs before merging it blindly without any tests running to v3--test similarly as we usually do for It literally makes no sense to merge such change when NONE of the tests were actually running. Merging it has no verification whatsoever, so you might as well wait with merging it until the issue in v3-1-test is fixed. I could understand if there is a "known" issue and our PR has unrelated errors - but in this cases no tests were running at all. And In this case it was pretty clear that the offending PR was #56208 (that was first that failed in v3-1-test branch (previous one was green - this is quite visible here: When you look at things merged to v3-1-test *https://github.com/apache/airflow/commits/v3-1-test/ You can see that in #56208 number of tests run dropped dramatically:
So in this case, getting rid of the big issue was as easy as reverting the #56208. I think otherwise, release manager might have really difficult job to chase and fix all the issues instroduced by no test run at all. |
|
I understand. I didn't have time to look into it at the time, so it was either merge it because failure were unreleated (and I'm confident about the change introduced here), or do nothing and also add work to release manager because he would have to cherry pick much more. I figured merging was more helpful, also if I recall correctly the release process, we cherry pick to the test branch manually without running the CI at all, and fix the CI at the end when we do the sync PR with stable branch. (but maybe that's reserved to the RM?) But I'm also fine letting the PRs open, or closing them and let the RM to the cherry picking later when the backport CI is fixed. (With all that implies, letting them open until someone actually do something about it by specifically targeting those PRs. (I usually don't but will have to if we want to work like this)). |
This also effectively runs PR on schedule now (few times a day) and when things fail, the errors are posted to #internal-airflow-ci-cd. This was happening continuously - see alerts here for example https://apache-airflow.slack.com/archives/C015SLQF059/p1759424572437079 - this is what dragged my attention, I saw that those failures were continuously happening day after day over last few days. |
That often takes an order of magnitude more effort, because you have many failures and you have no idea were they came from. Which is exactly what I am afraid and try to protect the time of release manager who should generally not chase issues from days or weeks ago, I think (that's more or less the effect and why we introduced the notifications in slack as well) Note that I am not criticising, just trying to show the mechanism "while it's hot" to get a little more empathising (with RM) and raise understanding / awareness why this is important. |
|
Also.... We talk about it with @amoghrajesh and @gopidesupavan (remotely) at the Summit ... so .... come to listen to us :) |

Extract component for expand and collapse button group
Refactor toggle groups buttons
(cherry picked from commit 6d1991d)
Co-authored-by: Pierre Jeambrun pierrejbrun@gmail.com