.github/workflows/build.yml, doc-build.yml: Fix get_ci_fixes #36348
Conversation
|
@tobiasdiez Seems you didn't see #36338 (comment) |
mkoeppe
force-pushed
the
fix_get_ci_fixes
branch
from
97bfb3a
to
c5599a3
Compare
|
@tobiasdiez Kindly refrain from pushing untested changes to automatically applied blocker PRs. |
Changes to CI workflows are not applied anyway...but yeah, another disadvantage of using the single blocker label. Good that you see this yourself. |
|
Let's try to find a proper fix in #36349, that also resolves a few other issues with the auto-apply of PRs. We can leave this one open, to test if merging of other PRs works or not. |
|
This is a proper fix. |
|
Thanks, Dima! |
There was a problem hiding this comment.
Sorry, but I'm not happy with this. Please properly fix the issues mentioned in #36349. There is no need to hurry to get another hotfix in. Also, please test changes to these workflows when there are other blockers and when there are not.
|
The PR fixes what it promises to fix. |
|
Can you please not set it to positive review when there is a negative review. Thanks! |
|
Tobias, the change here has been tested and has been reviewed by an expert. I don't believe that you have actual concerns that my fix here fixes the described problem. As you refer to #36349, this PR fixes the first bullet point. |
|
Once again let me point you to https://github.com/sagemath/sage/blob/develop/CODE_OF_CONDUCT.md |
|
We have two PRs both fixing the same issue by (slightly) different means. It's only natural to decide first which of these approaches is the better one, before merging either of these PRs. For example, if I were to propose a new PR that would simply disable the "get ci fixes" functionality for now, you would probably dismiss this PR as well - although it would fix the bug as well. |
Really? This PR (M) fixes the issue (no blocker PRs), and does only that. Your PR (T) takes back steps, removing the issue, and does more things. If we take (T), we discard (M). But if we take (M), we may still consider (T) and can take (T) or updated (T) (as it does a few things currently). Taking (M) does not mean discarding (T). If you block this PR (M), then you are forcing us to take your PR (T). This is not a valid reason to block a PR. This is not a proper way to promote your PR. |
<!-- Please provide a concise, informative and self-explanatory title. --> <!-- Don't put issue numbers in the title. Put it in the Description below. --> <!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to multiply two integers" --> ### 📚 Description Follow-up on sagemath#35169. The container `sage-docker-fedora-31-maximal-with-targets` used for the PDF docbuild turned out to be not reliable. Here we replace it by `ubuntu-focal-standard-with-targets` and install texlive in it. We also copy over the incremental build from doc-build.yml and the method to get CI fixes from blocker tickets from sagemath#36338. This workflow is currently disabled in sagemath/sage. Example run: https://github.com/mkoeppe/sage/actions/runs/6318741659/job/17158468016 <!-- Describe your changes here in detail. --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> - Depends on sagemath#36338 - Depends on sagemath#36348 <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#35373 Reported by: Matthias Köppe Reviewer(s): Dima Pasechnik
…_ci_fixes
... for the case when there are no blocker PRs to be applied
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36348
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Kwankyu Lee, Matthias Köppe, Tobias Diez
|
@kwankyu I'm deleting the code that Matthias is changing. So no, we cannot have both fixes at the same time in the codebase. Please, please refrain from putting the positive review label on if there are clearly changes requested. Didn't we learn from the whole Python drop debacle that label wars are not a nice thing (for both sides)? It's probably to late now but @vbraun please remove this commit again from your merge branch. Let's hope this is actually working, since I couldn't find a test run without any blocker tickets...which, in my opinion, should have been for a prerequisite for a review. |
|
Tobias, you are abusing your privileges as a member of the Triage role with these obstructive actions. I'll refer this to sage-abuse (again). |
Once again, Tobias, I don't believe you that you actually have these concerns. |
Well, last time I remarked that this auto-fix feature was not well tested (in my opinion), it broke many CI workflows. So, yes, you can believe me that I have the concern that an (what appears to me as an) untested change to the CI infrastructure breaks things again. Did you actually run the workflow without any (other) PRs marked as blocker or not? |
I also have to call out this inappropriate attempt to intimidate other reviewers, and this new attempt to gaslight others regarding our project's procedures and standards. No, the members of our project did not need the learning experience from your abusive conduct in #35404 (and related tickets). The standard in our community is to use technical and civilized discussions made in good faith. Tobias, it's not too late to change how you act in our community, and I hope that you can make this necessary change. |
<!-- Please provide a concise, informative and self-explanatory title. --> <!-- Don't put issue numbers in the title. Put it in the Description below. --> <!-- For example, instead of "Fixes #12345", use "Add a new method to multiply two integers" --> ### 📚 Description Follow-up on #35169. The container `sage-docker-fedora-31-maximal-with-targets` used for the PDF docbuild turned out to be not reliable. Here we replace it by `ubuntu-focal-standard-with-targets` and install texlive in it. We also copy over the incremental build from doc-build.yml and the method to get CI fixes from blocker tickets from #36338. This workflow is currently disabled in sagemath/sage. Example run: https://github.com/mkoeppe/sage/actions/runs/6318741659/job/17158468016 <!-- Describe your changes here in detail. --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes #12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - #12345: short description why this is a dependency - #34567: ... --> - Depends on #36338 - Depends on #36348 <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: #35373 Reported by: Matthias Köppe Reviewer(s): Dima Pasechnik
mkoeppe
added
the
disputed
... for the case when there are no blocker PRs to be applied
📝 Checklist
⌛ Dependencies