Re: [PR] github: Setup protection for master branch. [nuttx]
cederom commented on PR #16324: URL: https://github.com/apache/nuttx/pull/16324#issuecomment-2858252790 Another problem I can see is "Commits must have verified signatures.". Looks like we need to update `required_signatures: false` too as signature in github nomenclature means cryptographic signature not just `git commit -s` o_O https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] github: Setup protection for master branch. [nuttx]
cederom commented on PR #16324: URL: https://github.com/apache/nuttx/pull/16324#issuecomment-2858241271 You are right @raiden00pl with this `strict: true` all PR needs to be rebased each time master is updated that is a waste of focus and CI resources, sorry, the nomenclature is a bit misleading, the fix that sets `strict: false` is here https://github.com/apache/nuttx/pull/16336 :-) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] github: Setup protection for master branch. [nuttx]
cederom commented on PR #16324: URL: https://github.com/apache/nuttx/pull/16324#issuecomment-2858163302 > @raiden00pl: dependency on `Build` seems bad to me. With this change it becomes impossible to merge changes where `nuttx` and `nuttx-apps` need to be changed at the same time. > > Also `strict: true` requires some more consideration. If I understand correctly, all PRs must now be synced with master. This will cause much more effort for the committers because it will require constant synchronization with the master. As a consequence, CI will be much more loaded due to constant rebases to master. Thank you @raiden00pl! We will see how it works and update when needed! :-) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] github: Setup protection for master branch. [nuttx]
raiden00pl commented on PR #16324: URL: https://github.com/apache/nuttx/pull/16324#issuecomment-2857455486 dependency on `Build` seems bad to me. With this change it becomes impossible to merge changes where `nuttx` and `nuttx-apps` need to be changed at the same time. Also `strict: true` requires some more consideration. If I understand correctly, all PRs must now be synced with master. This will cause much more effort for the committees because it will require constant synchronization with the master. As a consequence, CI will be much more loaded due to constant rebases to master. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] github: Setup protection for master branch. [nuttx]
xiaoxiang781216 merged PR #16324: URL: https://github.com/apache/nuttx/pull/16324 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] github: Setup protection for master branch. [nuttx]
cederom commented on PR #16324: URL: https://github.com/apache/nuttx/pull/16324#issuecomment-2855940539 Okay I have asked #asfinfa on Slack, @Humbedooh said this looks okay, in case we break anything it should be possible to unbreak :D Switching to "ready for review" and turning on cypress in my parachute :D -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] github: Setup protection for master branch. [nuttx]
pussuw commented on PR #16324: URL: https://github.com/apache/nuttx/pull/16324#issuecomment-2854725871 So linear history means fast forward only. Yes, enforce that please. I reading hate a git history that has a million divergent branches. Impossible to follow. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] github: Setup protection for master branch. [nuttx]
cederom commented on PR #16324: URL: https://github.com/apache/nuttx/pull/16324#issuecomment-2854665017 > @pussuw: What does linear history mean, no merge commits allowed ? Good question :-) Admins help would be nice here :-P I think we DO WANT the linear history as we do rebase PRs not fast foward.. but for some reason [1] has this set to false. Quick search reveals linear history keeps things clean and we should want that :-) https://dev.to/bladesensei/avoid-messy-git-history-3g26 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] github: Setup protection for master branch. [nuttx]
pussuw commented on PR #16324: URL: https://github.com/apache/nuttx/pull/16324#issuecomment-2854632952 What does linear history mean, no merge commits allowed ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] github: Setup protection for master branch. [nuttx]
nuttxpr commented on PR #16324: URL: https://github.com/apache/nuttx/pull/16324#issuecomment-2854570505 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR likely meets the NuttX requirements, but needs some adjustments to fully comply. Here's a breakdown: **Strengths:** * **Clear Summary:** Explains the "why" (missing settings), "what" (.asf.yaml update), and "how" (specific settings being changed). The link to the reference is helpful. * **Acknowledges Impact:** Correctly identifies the impact as changes to the master branch protection. * **Honest Testing:** Acknowledges the lack of complete familiarity and requests review, which is good practice. Marking as a draft is also appropriate. **Weaknesses & Suggestions for Improvement:** * **Summary - Be More Specific:** While the summary lists the settings being changed, it needs more detail about *how* they work. For example, instead of "Require status checks to pass before merge," specify *which* status checks. Instead of "Setup reviews parameters," state the *specific* review requirements (e.g., minimum number of reviewers). * **Impact - Detail Changes:** Expand on the impact. How will branch protections affect developers? Will it change their workflow? This section should be more user-centric. For example, mention how the changes prevent accidental pushes to master and enforce code review. * **Testing - Insufficient Logs:** The "Testing" section lacks concrete testing logs. While fully testing these settings might be difficult before merging, you can still provide *examples* of how you expect the settings to behave. For instance, show the expected error message if someone tries to push directly to master. Alternatively, if you have a test repository you used to experiment, include logs from that. * **Testing - Clarify Check Names:** Follow up with @lupyuen and @hartmannathan about the check names *before* requesting review from Apache Admins. This will streamline the review process. * **Linear History:** Make a decision about enforcing linear history. Research the pros and cons and explain your reasoning in the PR description. **Specific Action Items:** 1. **Detail the settings changes in the Summary.** Provide concrete values and explain how each setting will function. 2. **Expand the Impact section.** Focus on the user perspective and explain how the changes will affect their workflow. 3. **Improve the Testing section.** Either provide example logs demonstrating expected behavior or explain why testing is limited at this stage. If possible, test in a controlled environment. 4. **Confirm the check names.** Contact @lupyuen and @hartmannathan. 5. **Decide about linear history.** Explain your reasoning in the PR. By addressing these points, your PR will be much stronger and more likely to be accepted quickly. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
