fix: Set success Git commit status in case of skipped checks

[andridzi]

No description provided.

[djeebus]

Good catch, thanks! Mind taking a shot at implementing it for gitlab as well?

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/vcs/github_client/client.go

@@ -181,7 +181,7 @@ func toGithubCommitStatus(state pkg.CommitState) *string {
 		return pkg.Pointer("failure")
 	case pkg.StateRunning:
 		return pkg.Pointer("pending")
-	case pkg.StateSuccess, pkg.StateWarning, pkg.StateNone:
+	case pkg.StateSuccess, pkg.StateWarning, pkg.StateNone, pkg.StateSkip:
 		return pkg.Pointer("success")
 	}
 

Feedback & Suggestions:

1. Enum Handling: The addition of pkg.StateSkip to the case statement is a good step to ensure that all possible states are handled. However, ensure that pkg.StateSkip is indeed intended to map to a "success" status. If StateSkip represents a different logical outcome, consider mapping it to a more appropriate status.

2. Documentation: Consider updating any relevant documentation or comments to reflect the addition of pkg.StateSkip. This will help maintain clarity for future developers who might work on this code.

3. Testing: Ensure that there are corresponding test cases for this new state to verify that the function behaves as expected when pkg.StateSkip is encountered.

---

😼 Mergecat review of pkg/vcs/gitlab_client/status.go

@@ -69,7 +69,7 @@ func convertState(state pkg.CommitState) gitlab.BuildStateValue {
 		return gitlab.Running
 	case pkg.StateFailure, pkg.StateError, pkg.StatePanic:
 		return gitlab.Failed
-	case pkg.StateSuccess, pkg.StateWarning, pkg.StateNone:
+	case pkg.StateSuccess, pkg.StateWarning, pkg.StateNone, pkg.StateSkip:
 		return gitlab.Success
 	}
 

Feedback & Suggestions:

1. State Mapping Clarity: The addition of pkg.StateSkip to return gitlab.Success might be intentional, but it's important to ensure that this mapping aligns with the intended logic of your application. If StateSkip should indeed map to a successful state, then this change is correct. However, if StateSkip should represent a different state, consider mapping it to a more appropriate gitlab.BuildStateValue.

2. Documentation Update: If pkg.StateSkip is a new addition, ensure that any relevant documentation or comments are updated to reflect this change. This will help maintain clarity for future developers working on this code.

3. Testing: Ensure that there are adequate tests covering this new state to verify that it behaves as expected in all scenarios. This will help catch any unintended side effects from this change.

---

😼 Mergecat review of pkg/commitState.go

@@ -30,6 +30,7 @@ func (s CommitState) BareString() string {
 
 var stateString = map[CommitState]string{
 	StateNone:    "",
+	StateSkip:    "Skipped",
 	StateSuccess: "Passed",
 	StateRunning: "Running",
 	StateWarning: "Warning",
@@ -62,6 +63,8 @@ func ParseCommitState(s string) (CommitState, error) {
 		return StateError, nil
 	case "panic":
 		return StatePanic, nil
+	case "skip", "skipped":
+		return StateSkip, nil
 	default:
 		return StateNone, fmt.Errorf("unknown commit state: %s", s)
 

Feedback & Suggestions:

1. Consistency in State Mapping: The addition of StateSkip to the stateString map and the ParseCommitState function is a good improvement for consistency. Ensure that all states are consistently represented across the codebase to avoid any discrepancies.

2. Case Sensitivity Handling: The ParseCommitState function already uses strings.ToLower, which is great for handling case insensitivity. Ensure that any new states added in the future follow this pattern to maintain consistency.

3. Testing: Make sure to add or update unit tests to cover the new StateSkip case. This will help ensure that the changes work as expected and that the function handles all possible inputs correctly.

4. Documentation: Consider updating any relevant documentation or comments to reflect the addition of the StateSkip state. This will help maintain clarity for future developers working with this code.

Overall, the changes are well-integrated and improve the functionality of the code. Keep up the good work! 🚀

---

---

Dependency Review

Click to read mergecats review!

No suggestions found

[andridzi]

sure, done
gitlab part completely went out of my head

[andridzi]

hello @djeebus
what is your release/merge strategy, any chance this one can be processed?

[djeebus]

hello @djeebus what is your release/merge strategy, any chance this one can be processed?

"Strategy" is a strong word, mostly we release when someone pokes us. Release coming!

[djeebus]

v1.8.1, thanks!