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

allow branches #34

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

till
Copy link

@till till commented Feb 5, 2023

  • Chore: upgrade go to 1.19
  • Chore: update actions
  • Update: allow push to certain branches

@till till mentioned this pull request Feb 5, 2023
Copy link
Author

@till till left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments to share my thoughtprocess.

@@ -4,7 +4,7 @@ on:
- push

env:
GO_VERSION: 1.18
GO_VERSION: 1.19
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly updated to be able to use some functions and also, newer is better. :D

@@ -14,12 +14,12 @@ jobs:
steps:

- name: Checkout code
uses: actions/checkout@v2
uses: actions/checkout@v3
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just updated these as older actions will stop working eventually (some nodejs thing that GitHub deprecated).

@@ -45,14 +47,30 @@ func IsForcePush(hook *HookInfo) (bool, error) {
return base != hook.OldRev, nil
}

func (r *Receiver) CheckAllowedBranch(hook *HookInfo) error {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this public so I could add a test. Otherwise, the code in Handle requires a full blown setup with commits and repositories and so on. Not ideal, but hope it's alright.

receiver.go Outdated
@@ -45,14 +47,30 @@ func IsForcePush(hook *HookInfo) (bool, error) {
return base != hook.OldRev, nil
}

func (r *Receiver) CheckAllowedBranch(hook *HookInfo) error {
if r.MasterOnly { // for BC
r.AllowedBranches = append(r.AllowedBranches, "refs/heads/master")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured comparing to the actual names, saved some parsing. But let me know if you'd like another API with straight up branch names like []string{"master","main","etc"} instead.

@till
Copy link
Author

till commented Feb 18, 2023

@sosedoff Can you take a look some time?

@sosedoff
Copy link
Owner

sosedoff commented Mar 1, 2023

Yea will review this week.

Copy link
Owner

@sosedoff sosedoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far, see my comments and adjust accordingly.

receiver.go Outdated Show resolved Hide resolved
receiver_test.go Outdated Show resolved Hide resolved
receiver_test.go Outdated Show resolved Hide resolved
till added 3 commits June 16, 2023 17:30
This extends MasterOnly and makes it configurable. The idea is to
set arbitrary branch names which are allowed to be pushed to and
otherwise return an error.

Resolves: sosedoff#33
@till till force-pushed the allow-branches branch from e2a92c3 to 1d2f53d Compare June 16, 2023 15:30
- renamed AllowedBranches to AllowedRefs
- use t.Run() in tests
@till till force-pushed the allow-branches branch from 0d2ec1d to 17559cf Compare June 16, 2023 15:40
@till
Copy link
Author

till commented Jun 16, 2023

@sosedoff this took me a bit longer than expected, can you have a look again?

@till till mentioned this pull request Jun 18, 2023
@till
Copy link
Author

till commented Apr 22, 2024

@sosedoff any interest in this?

@sosedoff
Copy link
Owner

Probably, i haven't used the repo myself in quite a bit so getting up to speed will take some time

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

Successfully merging this pull request may close these issues.

2 participants