Multiple changes in one PR #27411
-
|
After reading #26935 (comment), an idea came to my mind that if we can make a community moderator from #15674 (comment), a head moderator or give them additional permissions so that they can merge PRs like #26935 or PRs containing multiple small changes (like updating ARP or monikers). This will help to reduce the workload to approve multiple PRs for the same change. For example: when I raised issue #21256, Levvie had to open 400+ PRs and giving additional permissions to moderators will be helpful in these cases. Mentioning @ImJoakim @ItzLevvie @jedieaston @KaranKad @OfficialEsco @quhxl @denelon to give their opinions. |
Beta Was this translation helpful? Give feedback.
Replies: 4 comments 9 replies
-
|
Improving validation process will also help with this issue. eg -
|
Beta Was this translation helpful? Give feedback.
-
|
In addition to this, a contributor should be allowed to move or delete (not add) .validation files. A check should be done before that no version exists inside that Package Identifier. |
Beta Was this translation helpful? Give feedback.
-
|
@denelon can you put this in the backlog? |
Beta Was this translation helpful? Give feedback.
-
|
We're driving critical behavior with .validation files. They will continue to require one of the core team members to be removed/added. |
Beta Was this translation helpful? Give feedback.
-
|
ARP changes probably need to be tested, to make sure matching works. The rest of the metadata is probably safe to not test IMO. I think we've discussed having multiple manifests in one PR before (I don't have a citation for that though), but the problem is that it makes it too easy to sneak something in that could cause issues. The pipelines are set up to only test one manifest at a time right now, they'd probably have to be adjusted a lot. I think there's logic in having a break glass "oh crap we have to change the license for all instances of Docker Desktop" flow, but approvals should probably be restricted to people at MS. It shouldn't need to be used very often, instead we should continue finding ways to automate checking manifests when they are added (ARP stuff could be entirely automated, as an example). |
Beta Was this translation helpful? Give feedback.
-
|
Well, I think multiple manifests is slightly different from multiple changes. When testing multiple manifests in one PR, the logic becomes too complex and relatively slow. But, as KaranKad said, we can allow changes like metadata and deleting multiple versions of packages in one PR. |
Beta Was this translation helpful? Give feedback.
-
Isn't that multiple manifests? |
Beta Was this translation helpful? Give feedback.
-
|
Deleting manifests doesn't need any tests like ARP and Manifest Content Validation as most of the time we are deleting them because they are not working properly. The InstallerUrl is 404 or the application does not install silently. What I think about multiple manifests is when we add/update packages in one PR. |
Beta Was this translation helpful? Give feedback.
-
|
We're still working through the implications of multiple manifests in a single PR. There are multiple edge cases. I've broken the publish pipeline at least once in the past when approving multiple deletes in one PR. It's still work in progress. |
Beta Was this translation helpful? Give feedback.
-
|
Is there an issue which is tracking against this at all, or one which has the disposition of whether or not this will be completed? I personally would like to give an upvote to the idea, but I understand that there are many complexities underlying this. If this is work that will be completed eventually, I'd also like to help review the spec |
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
|
Thank you! I dont know why I couldn't find it earlier |
Beta Was this translation helpful? Give feedback.
-
|
Good I saw this in discussions. I agree there should be a way out Vendor name change from Originally it was just one PR to move everything out from EclipseFoundation to EclipseAdoptium then based on CI pipeline failures:
Eventually I closed it to pick up later. That itself breaks the atomicity of the requirement. Wouldn't make much sense if one would look back at history as well |
Beta Was this translation helpful? Give feedback.
We're still working through the implications of multiple manifests in a single PR. There are multiple edge cases. I've broken the publish pipeline at least once in the past when approving multiple deletes in one PR. It's still work in progress.