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

Jump to bottom

Skip Checks API neutral state #278

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

Skip Checks API neutral state #278

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c1901c2
The neutral status cannot be determined as success or failure, so the…
yamachu Nov 15, 2021
c6d909a
Add method to determine CI is in progress or not
yamachu Nov 17, 2021
417f9ef
Separate Status and CheckSuite Event struct
yamachu Nov 28, 2021
9e5baff
Remove unused method
yamachu Nov 28, 2021
1769fde
Duplicate checkAutoBranch for each Event
yamachu Nov 28, 2021
b4c8892
The neutral status cannot be determined as success or failure, so the…
yamachu Nov 28, 2021
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
20 changes: 20 additions & 0 deletions epic/check_autobranch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type StateChangeInfo struct {
ID int64
SHA string
IsRelatedToAutoBranchBody func(string) bool
IsInProgress func() bool
Copy link
Contributor

Choose a reason for hiding this comment

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

@yamachu I imagined the following code instead of passing a function to .IsInProgress field.

type StateChangeInfo struct {
+    Conclusion string
}

+func IsInProgress(s *StateChangeInfo) bool {
+  ....
+}

What do you think about this? Are there any problem to implement?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tetsuharuohzeki In the code you were suggesting, did you imagine to set the Conclusion field to ev.State when received a StatusAPI event?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

Ah, but github.StatusEvent does not have Conclusion field. I think we can use *string for StateChangeInfo.Conclusion

Copy link
Contributor

Choose a reason for hiding this comment

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

@yamachu

Ah, but github.StatusEvent does not have Conclusion field. I think we can use *string for StateChangeInfo.Conclusion

I wrote that but I also seem your proposed code would also be nice approach which uses func.
You can choose the final implementation. How do you think?

Copy link
Member Author

@yamachu yamachu Nov 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It's a different types, but I think I can express it in a function like TypeScript's Union type, so I'll add a Conclusion *string parameter.

In the following comments, would such an implementation be appropriate?
https://github.com/voyagegroup/popuko/pull/278/files#r754050176

func isStatusDetermined(info StateChangeInfo) bool {
	// Status API
	if info.Conclusion == nil {
		return true // or info.Status != "pending"
	}

	// Checks API
	return *info.Conclusion != "neutral"
}

func isStatusSuccess(info StateChangeInfo) bool {
	// Status API
	if info.Conclusion == nil {
		return info.Status == "success"
	}

	// Checks API
	return *info.Conclusion == "success"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@yamachu

Umm, I agree that we would need to define a status by something.

However, from your proposed psuedo code, I think the current design is wrong to handle these status.
Your proposed one mixes handling Status API and Checks API in the same code path. This makes a thing more complex.

I think we would require these refactoring to introduce this PR change cleanly:

  1. Introduce interface type which abstracts StateChangeInfo, and separate a code path into for Status API and Checks API.
  2. Do something which we would like to do in this pull request.

Copy link
Member Author

Choose a reason for hiding this comment

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

私は英語ネイティブではないため、コメントに対してコストがかかってしまっています。
そのためコメントを日本語で記載いたします。

異なる概念であるStatus APIのEventとChecks APIのEventを同一の構造体で表現していることで複雑になっていることは同意いたします。
例えば info.Status で表現されている内容が、ChecksAPIの場合はConclusionの値であるといった例です。

そのためコードパスを分離することは確かに良いように思われます。
Event依存の構造体に非依存の構造体を入れ込み、Event依存のコードパスを分離することでまずは表現可能だと考え、以下のコミットで表現してみました。
1769fde

非依存の構造体に例えば IsRelatedToAutoBranchBody のように、関数を返す形にすることで

log.Println("info: the status event is related to auto branch.")

info.mergeSucceedItem(ctx, client, info.Owner, info.Name, repoInfo, q)

q.RemoveActive()

の様にエントリポイントである checkAutoBranch は共通化出来そうではありますが、この構造体に持たせるのは違和感がありました。
そのため横に別名で生やすことで、コードの分離を実現しています。

}

func CheckAutoBranchWithStatusEvent(ctx context.Context, client *github.Client, autoMergeRepo *queue.AutoMergeQRepo, ev *github.StatusEvent) {
Expand All @@ -32,6 +33,7 @@ func CheckAutoBranchWithStatusEvent(ctx context.Context, client *github.Client,
ID: *ev.ID,
SHA: *ev.SHA,
IsRelatedToAutoBranchBody: isRelatedToAutoBranchBodyWithStatusEvent(ev),
IsInProgress: inProgressWithStatusEvent(ev),
}
checkAutoBranch(ctx, client, autoMergeRepo, info)
}
Expand All @@ -46,6 +48,7 @@ func CheckAutoBranchWithCheckSuiteEvent(ctx context.Context, client *github.Clie
ID: *ev.CheckSuite.ID,
SHA: *ev.CheckSuite.HeadSHA,
IsRelatedToAutoBranchBody: isRelatedToAutoBranchBodyWithCheckSuiteEvent(ev),
IsInProgress: inProgressWithCheckSuiteEvent(ev),
}
checkAutoBranch(ctx, client, autoMergeRepo, info)
}
Expand Down Expand Up @@ -130,6 +133,18 @@ func isRelatedToAutoBranchBodyWithCheckSuiteEvent(ev *github.CheckSuiteEvent) fu
}
}

func inProgressWithStatusEvent(ev *github.StatusEvent) func() bool {
return func() bool {
return *ev.State == "pending"
}
}

func inProgressWithCheckSuiteEvent(ev *github.CheckSuiteEvent) func() bool {
return func() bool {
return *ev.CheckSuite.Status != "completed" || *ev.CheckSuite.Conclusion == "neutral"
Copy link
Contributor

Choose a reason for hiding this comment

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

@yamachu

As your comment, I feel *ev.CheckSuite.Conclusion == "neutral" does not mean in progress state directly.

But this code will check whether *ev.CheckSuite.Conclusion is neutral after if *ev.CheckSuite.Status != "completed" equals to that *ev.CheckSuite.Status indicates some stopped states.

I concern that this function will return true in almost case. Is this right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I concern that this function will return true in almost case. Is this right?

Yes, it enforces the order of use, which doesn't seem to be a good function...
Rather, what should be expressed is not whether it is in progress or not, but whether the state is determined or not.

}
}

func isRelatedToAutoBranch(active *queue.AutoMergeQueueItem, info StateChangeInfo, autoBranch string) bool {
if !info.IsRelatedToAutoBranchBody(autoBranch) {
log.Printf("warn: this event (%v) is not the auto branch\n", info.ID)
Expand Down Expand Up @@ -183,6 +198,11 @@ func mergeSucceedItem(
return true
}

if info.IsInProgress() {
log.Printf("info: Check is in progres\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: progress?

return true
}

if info.Status != "success" {
log.Println("info: could not merge pull request")

Expand Down