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

Support some changes without involving EF #110

Closed
eclipse-csi-bot opened this issue Sep 15, 2023 · 10 comments
Closed

Support some changes without involving EF #110

eclipse-csi-bot opened this issue Sep 15, 2023 · 10 comments

Comments

@eclipse-csi-bot
Copy link
Contributor

In GitLab by @jograham on Sep 15, 2023, 15:51

I have a use case as mentioned in the recent video presentation about otterdog.

I want to enable branch protection to prevent accidental pushes to main for "normal" code changes. But during release cycles I need to be able to push releng changes directly to main and at least not have reviews needed. At the moment we have no branch protection as the workaround and we revert if we accidentally screw up and push to main prior to review.

This tends to affect projects that have few active committers.

@eclipse-csi-bot
Copy link
Contributor Author

In GitLab by @jograham on Sep 15, 2023, 15:55

The desire here is to be able to override the branch protection rules for a single push or set of pushes, but the timeline for this generally means I want to do this quickly and avoid the turnaround time with helpdesk.

One idea for how otterdog could help here is if there were multiple files (e.g. two versions of https://github.com/eclipse-cdt-cloud/.eclipsefdn/blob/main/otterdog/eclipse-cdt-cloud.jsonnet) that have been approved by EF staff and I as PL (or committer) can swap between them automatically (i.e. without helpdesk involvement) that would probably work.

Of course I may be missing some better options of how to handle this, e.g. maybe GitHub supports what I want already, but if so I haven't been able to figure it out.

@eclipse-csi-bot
Copy link
Contributor Author

In GitLab by @netomi on Sep 15, 2023, 16:36

Just trying to think out of the box. Are only specific people required to do direct pushes to the main branch during a release cycle?

Would adding these people to a bypass list already be a workable solution? So in general you have a branch protection rule in place that e.g. force you to create a PR prior to merge changes, but some people (e.g. project leads can bypass this rule when needed).

@eclipse-csi-bot
Copy link
Contributor Author

In GitLab by @jograham on Sep 15, 2023, 16:41

A bypass list would be a step in the right direction, i.e. I do all the releng on CDT (normally) so I could be in bypass list. But really my non-releng changes should go through PRs/reviews ideally and it would be nice if I could prevent accidental pushes to main.

@eclipse-csi-bot
Copy link
Contributor Author

In GitLab by @netomi on Sep 15, 2023, 19:40

Having thought about it, I think the cleanest solution to that would be to let the necessary commits for a release be performed by the eclipse-bot-account via a workflow that e.g. is triggered by creating a draft release (or anything else that would indicate the release).

I recently worked on a similar workflow: https://github.com/eclipse-cbi/macos-notarization-service/blob/main/.github/workflows/release.yml that is triggered by pushing a tag to the repo. It then makes a build and upload the resulting artifacts to a prepared release.

I dont know what steps are required for the referenced repos to be pushed directly to the main branch, maybe I can help with working on such a workflow, I got a lot of experience with GitHub actions lately. If you are interested we can setup a call to dicuss that.

@eclipse-csi-bot
Copy link
Contributor Author

In GitLab by @netomi on Sep 19, 2023, 08:56

btw, when doing a squash merge for a pull request you should also be able to have a tag matching the respective commit imho unless I miss something.

A usable setup for small projects with very few committers is to require a PR but set the number of approvals to 0. This would prevent accidental commits and still allow you to commit quickly if needed imho. Wdyt?

@eclipse-csi-bot
Copy link
Contributor Author

In GitLab by @jograham on Sep 19, 2023, 20:13

btw, when doing a squash merge for a pull request you should also be able to have a tag matching the respective commit imho unless I miss something.

You can tag after the squash & merge - but the flow I am referring to is "yarn publish" which bumps version number (in a commit) and tags that commit with the version number and publishes the content to npmjs. But... the right solution in this case is certainly to have that run as a workflow.

A usable setup for small projects with very few committers is to require a PR but set the number of approvals to 0. This would prevent accidental commits and still allow you to commit quickly if needed imho. Wdyt?

Sure, can you try setting that up for https://eclipse-cdt.github.io/.eclipsefdn/repo-cdt-infra/ and I can experiment a little with it and then roll it out to other repos? I thought we had some such rules already, but it doesn't prevent pushing directly, it just causes a warning like this:

$ git push origin HEAD 
Enumerating objects: 19, done.
Counting objects: 100% (19/19), done.
Delta compression using up to 12 threads
Compressing objects: 100% (12/12), done.
Writing objects: 100% (13/13), 1.28 KiB | 1.28 MiB/s, done.
Total 13 (delta 8), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (8/8), completed with 5 local objects.
remote: Bypassed rule violations for refs/heads/main:
remote: 
remote: - Changes must be made through a pull request.
remote: 
To github.com:eclipse-cdt/cdt.git
   147332763b..a498b56c59  HEAD -> main

@eclipse-csi-bot
Copy link
Contributor Author

In GitLab by @netomi on Sep 19, 2023, 20:52

I have created PR eclipse-cdt/.eclipsefdn#5 to apply the changes for the cdt-infra repo.

I have checked the cdt repo, there the rule is already setup like that:

  • requires a PR
  • set approval count to 0
  • includes yourself in the bypass list

thats the reason you could push to the main branch without PR

@Release workflow: I would be happy to help, just recently I worked on a release workflow for the notarization service: https://github.com/eclipse-cbi/macos-notarization-service/blob/main/.github/workflows/release.yml

and I got now a lot of experience with GitHub workflows.

@eclipse-csi-bot
Copy link
Contributor Author

In GitLab by @jograham on Sep 19, 2023, 21:09

I would be happy to help

That is great - I may come back to you soon on that one. The cdt-gdb-adapter has a fidly process that would be nice to automate. I'll start and ask you questions if I get stuck. (But it is lower priority so I will probably dip in and out of it over the coming weeks)

@eclipse-csi-bot
Copy link
Contributor Author

In GitLab by @jograham on Sep 19, 2023, 21:22

I have checked the cdt repo, there the rule is already setup like that:

I just tried it out and it seemed to do exactly what I wanted - i.e reject a direct push:

$ git push origin HEAD 
Enumerating objects: 23, done.
Counting objects: 100% (23/23), done.
Delta compression using up to 12 threads
Compressing objects: 100% (12/12), done.
Writing objects: 100% (13/13), 1.22 KiB | 1.22 MiB/s, done.
Total 13 (delta 7), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (7/7), completed with 5 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: Changes must be made through a pull request. Required status check "eclipsefdn/eca" is expected.
To github.com:eclipse-cdt/cdt-infra.git
 ! [remote rejected] HEAD -> master (protected branch hook declined)
error: failed to push some refs to 'github.com:eclipse-cdt/cdt-infra.git'

allowed me to create a PR: eclipse-cdt/cdt-infra#64, and once PR was created and (I assume ECA passed) allowed me to push:

$ git push origin HEAD 
Enumerating objects: 23, done.
Counting objects: 100% (23/23), done.
Delta compression using up to 12 threads
Compressing objects: 100% (12/12), done.
Writing objects: 100% (13/13), 1.22 KiB | 1.22 MiB/s, done.
Total 13 (delta 7), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (7/7), completed with 5 local objects.
To github.com:eclipse-cdt/cdt-infra.git
   fa5ecac..7562163  HEAD -> master

Can you apply the same change to:

@netomi
Copy link
Contributor

netomi commented Mar 18, 2024

There is now an auto-merge feature that should support that use-case as well.

@netomi netomi closed this as completed Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants