Skip Checks API neutral state #278

yamachu · 2021-11-15T10:07:28Z

We use CircleCI for CI.
Recently it will returns a neutral state before state is determined by the CI.
In the current code, except for success, it is designed to be considered a failure.

popuko/epic/check_autobranch.go

Line 186 in 7108011

if info.Status != "success" {

I thought it would be better to skip the decision for neutral, which cannot be determined as success or failure, so I added that process.

Webhook payload

{
  "action": "completed",
  "check_suite": {
    "id":,
    "node_id": "",
    "head_branch": "master",
    "head_sha": "",
    "status": "completed", <- completed???? test is running...
    "conclusion": "neutral", <- 
    "url": "",
    "before": "",
    "after": "",
    "pull_requests": [

    ],
   ...
}

… process is skipped For example, CircleCI may often send it

tetsuharuohzeki · 2021-11-15T15:10:22Z

epic/check_autobranch.go

@@ -183,6 +183,11 @@ func mergeSucceedItem(
 return true
 }

+ if info.Status == "neutral" {


I just knew conclusion field now....

I think we should define StateChangeInfo.Conclusion and check it at here.

By #261, we fill StateChangeInfo.Status in two way:

popuko/epic/check_autobranch.go

Line 27 in c1901c2

Status: *ev.State,

popuko/epic/check_autobranch.go

Line 41 in c1901c2

Status: *ev.CheckSuite.Conclusion,

But github.StatusEvent.Status and github.StatusEvent.Conclusion are different field. It's time to stop to mix them into single field. Let's handle them separately.

github.StatusEvent.Status and github.StatusEvent.Conclusion are different field. It's time to stop to mix them into single field.

Yes, from code design point of view, that's a good idea.
However, some refactoring is considered necessary.

BTY, I'm sorry to postpone the issue, but how about the following changes.

- NotHandle bool + InProgress bool

- NotHandle: *ev.CheckSuite.Status != "completed", + InProgress: *ev.CheckSuite.Status != "completed" || *ev.CheckSuite.Conclusion == "neutral"

@yamachu

Even though, I suspect it's better to add StateChangeInfo.InProgress() method instead of your proposed approach.
It's nice abstraction to push something into the appropriate method. I think that changing struct field as your proposed is not robustness for the future.

Then we would need to change to hold values of github.StatusEvent.Status and github.StatusEvent.Conclusion in StateChangeInfo. This requires a similar refactoring as I told the above.

tetsuharuohzeki · 2021-11-18T05:13:08Z

epic/check_autobranch.go

@@ -20,6 +20,7 @@ type StateChangeInfo struct {
 ID int64
 SHA string
 IsRelatedToAutoBranchBody func(string) bool
+ IsInProgress func() bool


@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?

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

Yes.

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

@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?

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" }

@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:

Introduce interface type which abstracts StateChangeInfo, and separate a code path into for Status API and Checks API.

Do something which we would like to do in this pull request.

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

異なる概念である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 は共通化出来そうではありますが、この構造体に持たせるのは違和感がありました。
そのため横に別名で生やすことで、コードの分離を実現しています。

tetsuharuohzeki · 2021-11-22T08:24:40Z

epic/check_autobranch.go

@@ -183,6 +198,11 @@ func mergeSucceedItem(
 return true
 }

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


nit: progress?

tetsuharuohzeki · 2021-11-22T08:41:50Z

epic/check_autobranch.go

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


@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?

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.

No longer needed as the same processing will be done within each Event-dependent process

… process is skipped

tetsuharuohzeki · 2021-11-30T19:02:36Z

@yamachu Sorry, I might not be able to reply soon....

tetsuharuohzeki

@yamachu

Thank you for your patience. I seem your new changesets are nice.

Probably, you may want to squash these commits or reorganize commits. But I feel other points are no problems and ready to merge nearly.

tetsuharuohzeki · 2021-12-05T23:18:51Z

epic/check_autobranch.go

@@ -11,8 +11,8 @@ import (
 "github.com/voyagegroup/popuko/setting"
 )

+// FIXME: Name


@yamachu Would you fix this comment before merging this?

tetsuharuohzeki · 2022-07-29T12:04:35Z

@yamachu Do you still face this problem?

The neutral status cannot be determined as success or failure, so the…

c1901c2

… process is skipped For example, CircleCI may often send it

tetsuharuohzeki suggested changes Nov 15, 2021

View reviewed changes

Add method to determine CI is in progress or not

c6d909a

tetsuharuohzeki reviewed Nov 18, 2021

View reviewed changes

tetsuharuohzeki reviewed Nov 22, 2021

View reviewed changes

yamachu added 4 commits November 29, 2021 02:18

Separate Status and CheckSuite Event struct

417f9ef

Remove unused method

9e5baff

No longer needed as the same processing will be done within each Event-dependent process

Duplicate checkAutoBranch for each Event

1769fde

The neutral status cannot be determined as success or failure, so the…

b4c8892

… process is skipped

tetsuharuohzeki reviewed Dec 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip Checks API neutral state #278

Skip Checks API neutral state #278

yamachu commented Nov 15, 2021

tetsuharuohzeki Nov 15, 2021

yamachu Nov 16, 2021

tetsuharuohzeki Nov 16, 2021

tetsuharuohzeki Nov 18, 2021

yamachu Nov 18, 2021

tetsuharuohzeki Nov 18, 2021

tetsuharuohzeki Nov 19, 2021

yamachu Nov 25, 2021 •

edited

Loading

tetsuharuohzeki Nov 27, 2021

yamachu Nov 28, 2021

tetsuharuohzeki Nov 22, 2021

tetsuharuohzeki Nov 22, 2021

yamachu Nov 25, 2021

tetsuharuohzeki commented Nov 30, 2021

tetsuharuohzeki left a comment

tetsuharuohzeki Dec 5, 2021

tetsuharuohzeki commented Jul 29, 2022

Skip Checks API neutral state #278

Are you sure you want to change the base?

Skip Checks API neutral state #278

Conversation

yamachu commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamachu Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tetsuharuohzeki commented Nov 30, 2021

tetsuharuohzeki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tetsuharuohzeki commented Jul 29, 2022

yamachu Nov 25, 2021 •

edited

Loading