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.

Sign up for GitHub

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

feat(repo)!: change allow_<event> fields to an AllowEvents struct + DB use integer masking #314

Merged
merged 24 commits into from
Dec 8, 2023
Merged

feat(repo)!: change allow_<event> fields to an AllowEvents struct + DB use integer masking #314

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
e575361
iota
ecrupper Jun 20, 2023
b72b1d4
nested lib type
ecrupper Jun 23, 2023
e60a7b0
comments for getters and setters
ecrupper Jun 26, 2023
ce74b93
Merge branch 'main' into feat-iota-event-matrix
ecrupper Sep 15, 2023
2024a8c
feat(repo)change allow events fields to an AllowEvents struct
ecrupper Sep 15, 2023
9f13d3b
remove pull review event constant
ecrupper Sep 15, 2023
93ef999
create repo method for event allowed
ecrupper Sep 15, 2023
e6ab6d2
linter
ecrupper Sep 15, 2023
ce5a837
remove unused schedule actions struct
ecrupper Sep 15, 2023
6f28b6c
Merge branch 'main' into feat-iota-event-matrix
ecrupper Sep 18, 2023
236843d
Merge branch 'main' into feat-iota-event-matrix
wass3rw3rk Sep 18, 2023
0552636
Merge branch 'main' into feat-iota-event-matrix
ecrupper Sep 25, 2023
8a0ecc6
Merge branch 'main' into feat-iota-event-matrix
ecrupper Oct 4, 2023
6337c97
copyright lint changes
ecrupper Oct 4, 2023
ac853a9
Merge branch 'main' into feat-iota-event-matrix
ecrupper Oct 5, 2023
d68e551
remove temp files used for debugging some tests
ecrupper Oct 5, 2023
241f3f5
Merge branch 'main' into feat-iota-event-matrix
wass3rw3rk Oct 9, 2023
eb21040
Merge branch 'main' into feat-iota-event-matrix
ecrupper Oct 12, 2023
c394810
move event_actions into separate package
ecrupper Oct 19, 2023
df9416b
leave space for future implemented pull actions
ecrupper Oct 20, 2023
394e61f
Merge branch 'main' into feat-iota-event-matrix
ecrupper Nov 10, 2023
3be6442
add more PR actions and bring back legacy fields
ecrupper Nov 14, 2023
c1c834b
Merge branch 'main' into feat-iota-event-matrix
ecrupper Nov 29, 2023
e7bc0a6
Update constants/allow_events.go
ecrupper Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions constants/allow_events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright (c) 2023 Target Brands, Inc. All rights reserved.
ecrupper marked this conversation as resolved.
Show resolved Hide resolved
//
// Use of this source code is governed by the LICENSE file in this repository.

package constants

// Allowed repo events.
const (
AllowPushBranch = 1 << iota // 00000001 = 1
AllowPullOpen // 00000010 = 2
AllowPullEdit // 00000100 = 4
AllowPullSync // ...
AllowPushTag
AllowDeployCreate
AllowCommentCreate
AllowCommentEdit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A note on this: we can't shuffle these at any point if this were to be implemented. So, if a pull_request event with the action review_requested is something we support later, then it must go at the bottom.

Copy link
Member

@wass3rw3rk wass3rw3rk Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

seems like a good thing to add to comments in-line oops, you did already - didn't show in a diff view i was looking at for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

Can we introduce some kind of version bit (byte?) so that the entire thing can be invalidated in the future? For now, just include the bit. Then, the migration logic can be introduced when we bump that version, if we ever do so.

Copy link
Member

Choose a reason for hiding this comment

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

Since we're using Intl64, and so far this only uses 8 bits, So, the version info could actually be at the end of the 64 bits instead of the beginning. Then, we don't need to reserve any space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My hesitation with including a version is that it would require some sort of DB migration script that updates all existing repos every time we add a new event we support. Part of me wants to just go in and enumerate all possible webhook actions that Vela could theoretically subscribe to and just mark them as not implemented if they aren't. That way future enhancements would build off a clean order and it wouldn't change unless GitHub itself adds a new event / action worth implementing.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Let's leave room for all current GitHub events, but leave a _ for all of the unimplemented ones with a comment saying what that slot is reserved for. Maybe even leave a bit of a gap in each category to add more later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added space for future implementations for pull_request actions that seem potentially relevant to CI

Copy link
Member

Choose a reason for hiding this comment

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

There seems to be a lot of other possible webhook events:
https://docs.github.com/en/webhooks/webhook-events-and-payloads

How did you pick which of the pull_request actions deserved an entry here?

)
23 changes: 8 additions & 15 deletions database/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,7 @@ type Repo struct {
Private sql.NullBool `sql:"private"`
Trusted sql.NullBool `sql:"trusted"`
Active sql.NullBool `sql:"active"`
AllowPull sql.NullBool `sql:"allow_pull"`
AllowPush sql.NullBool `sql:"allow_push"`
AllowDeploy sql.NullBool `sql:"allow_deploy"`
AllowTag sql.NullBool `sql:"allow_tag"`
AllowComment sql.NullBool `sql:"allow_comment"`
AllowEvents sql.NullInt64 `sql:"allow_events"`
PipelineType sql.NullString `sql:"pipeline_type"`
PreviousName sql.NullString `sql:"previous_name"`
}
Expand Down Expand Up @@ -185,6 +181,11 @@ func (r *Repo) Nullify() *Repo {
r.Timeout.Valid = false
}

// check if the AllowEvents field should be false
if r.AllowEvents.Int64 == 0 {
r.AllowEvents.Valid = false
}

// check if the Visibility field should be false
if len(r.Visibility.String) == 0 {
r.Visibility.Valid = false
Expand Down Expand Up @@ -225,11 +226,7 @@ func (r *Repo) ToLibrary() *library.Repo {
repo.SetPrivate(r.Private.Bool)
repo.SetTrusted(r.Trusted.Bool)
repo.SetActive(r.Active.Bool)
repo.SetAllowPull(r.AllowPull.Bool)
repo.SetAllowPush(r.AllowPush.Bool)
repo.SetAllowDeploy(r.AllowDeploy.Bool)
repo.SetAllowTag(r.AllowTag.Bool)
repo.SetAllowComment(r.AllowComment.Bool)
repo.SetAllowEvents(library.NewEventsFromMask(r.AllowEvents.Int64))
repo.SetPipelineType(r.PipelineType.String)
repo.SetPreviousName(r.PreviousName.String)

Expand Down Expand Up @@ -320,11 +317,7 @@ func RepoFromLibrary(r *library.Repo) *Repo {
Private: sql.NullBool{Bool: r.GetPrivate(), Valid: true},
Trusted: sql.NullBool{Bool: r.GetTrusted(), Valid: true},
Active: sql.NullBool{Bool: r.GetActive(), Valid: true},
AllowPull: sql.NullBool{Bool: r.GetAllowPull(), Valid: true},
AllowPush: sql.NullBool{Bool: r.GetAllowPush(), Valid: true},
AllowDeploy: sql.NullBool{Bool: r.GetAllowDeploy(), Valid: true},
AllowTag: sql.NullBool{Bool: r.GetAllowTag(), Valid: true},
AllowComment: sql.NullBool{Bool: r.GetAllowComment(), Valid: true},
AllowEvents: sql.NullInt64{Int64: r.GetAllowEvents().ToDatabase(), Valid: true},
PipelineType: sql.NullString{String: r.GetPipelineType(), Valid: true},
PreviousName: sql.NullString{String: r.GetPreviousName(), Valid: true},
}
Expand Down
22 changes: 7 additions & 15 deletions database/repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ func TestDatabase_Repo_Nullify(t *testing.T) {
Clone: sql.NullString{String: "", Valid: false},
Branch: sql.NullString{String: "", Valid: false},
Timeout: sql.NullInt64{Int64: 0, Valid: false},
AllowEvents: sql.NullInt64{Int64: 0, Valid: false},
Visibility: sql.NullString{String: "", Valid: false},
PipelineType: sql.NullString{String: "", Valid: false},
}
Expand Down Expand Up @@ -154,6 +155,7 @@ func TestDatabase_Repo_Nullify(t *testing.T) {
func TestDatabase_Repo_ToLibrary(t *testing.T) {
// setup types
want := new(library.Repo)
e := library.NewEventsFromMask(1)

want.SetID(1)
want.SetUserID(1)
Expand All @@ -172,11 +174,7 @@ func TestDatabase_Repo_ToLibrary(t *testing.T) {
want.SetPrivate(false)
want.SetTrusted(false)
want.SetActive(true)
want.SetAllowPull(false)
want.SetAllowPush(true)
want.SetAllowDeploy(false)
want.SetAllowTag(false)
want.SetAllowComment(false)
want.SetAllowEvents(e)
want.SetPipelineType("yaml")
want.SetPreviousName("oldName")

Expand Down Expand Up @@ -307,6 +305,8 @@ func TestDatabase_Repo_Validate(t *testing.T) {
func TestDatabase_RepoFromLibrary(t *testing.T) {
// setup types
r := new(library.Repo)
e := new(library.Events)
e.SetPush(new(library.PushActions).FromMask(1))

r.SetID(1)
r.SetUserID(1)
Expand All @@ -325,11 +325,7 @@ func TestDatabase_RepoFromLibrary(t *testing.T) {
r.SetPrivate(false)
r.SetTrusted(false)
r.SetActive(true)
r.SetAllowPull(false)
r.SetAllowPush(true)
r.SetAllowDeploy(false)
r.SetAllowTag(false)
r.SetAllowComment(false)
r.SetAllowEvents(e)
r.SetPipelineType("yaml")
r.SetPreviousName("oldName")

Expand Down Expand Up @@ -364,11 +360,7 @@ func testRepo() *Repo {
Private: sql.NullBool{Bool: false, Valid: true},
Trusted: sql.NullBool{Bool: false, Valid: true},
Active: sql.NullBool{Bool: true, Valid: true},
AllowPull: sql.NullBool{Bool: false, Valid: true},
AllowPush: sql.NullBool{Bool: true, Valid: true},
AllowDeploy: sql.NullBool{Bool: false, Valid: true},
AllowTag: sql.NullBool{Bool: false, Valid: true},
AllowComment: sql.NullBool{Bool: false, Valid: true},
AllowEvents: sql.NullInt64{Int64: 1, Valid: true},
PipelineType: sql.NullString{String: "yaml", Valid: true},
PreviousName: sql.NullString{String: "oldName", Valid: true},
}
Expand Down
12 changes: 4 additions & 8 deletions item_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ func TestTypes_ToItem(t *testing.T) {
num := 1
num64 := int64(num)
str := "foo"
e := new(library.Events)

b := &library.Build{
ID: &num64,
RepoID: &num64,
Expand Down Expand Up @@ -55,10 +57,7 @@ func TestTypes_ToItem(t *testing.T) {
Private: &booL,
Trusted: &booL,
Active: &booL,
AllowPull: &booL,
AllowPush: &booL,
AllowDeploy: &booL,
AllowTag: &booL,
AllowEvents: e,
}
u := &library.User{
ID: &num64,
Expand Down Expand Up @@ -106,10 +105,7 @@ func TestTypes_ToItem(t *testing.T) {
Private: &booL,
Trusted: &booL,
Active: &booL,
AllowPull: &booL,
AllowPush: &booL,
AllowDeploy: &booL,
AllowTag: &booL,
AllowEvents: e,
},
User: &library.User{
ID: &num64,
Expand Down
Loading