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

feat!: step status reporting + fix pull request context overwrite bug #1090

Merged
merged 7 commits into from
Mar 29, 2024

Conversation

ecrupper
Copy link
Contributor

@ecrupper ecrupper commented Mar 21, 2024

Closes go-vela/community#958

Adding the following lines to scm.Status(...):

	// only report opened, synchronize, and reopened action types for pull_request events
	if strings.EqualFold(b.GetEvent(), constants.EventPull) && !strings.EqualFold(b.GetEventAction(), constants.ActionOpened) &&
		!strings.EqualFold(b.GetEventAction(), constants.ActionSynchronize) && !strings.EqualFold(b.GetEventAction(), constants.ActionReopened) {
		return nil
	}

This will prevent any overwrites of the "core" pull_request event. Perhaps this can be a candidate for revisiting if we cut over to a version: "2" pipeline implementation, but the expectation of our users is that webhook events report to their respective contexts to the SCM. Therefore the behavior of continuous-integration/vela/pull_request should remain unchanged.

Closes go-vela/community#576

In order to still grant status reporting for "non-core" action types, opening the door for step reporting seemed like the right approach. With this change, steps with report_as defined will post their status to the defined context. It will be prefixed by the standard event prefix. For example:

  # validate title of PR on open or edit
  - name: validate title
    image: alpine:latest
    report_as: title-validator
    ruleset:
      event: [ pull_request:opened, pull_request:reopened, pull_request:edited ]
    commands:
      echo $VELA_BUILD_MESSAGE | grep -Eq "Expectation" && exit 0 || exit 1

This step will create and update a commit status with the context continuous-integration/vela/pull_request/title-validator.

The limitations I decided to impose are:

  1. Limit of 10 custom statuses per pipeline. This prevents SCM API abuse + approaching of the 1000 limit for status updates on a single commit.
  2. No reporting to the same context. This can only lead to problems and unexpected behavior.

Question I had: should a skipped step be a success?

go.mod Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 55.43478% with 41 lines in your changes are missing coverage. Please review.

Project coverage is 63.24%. Comparing base (8a5955d) to head (4bc0606).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1090      +/-   ##
==========================================
+ Coverage   63.12%   63.24%   +0.11%     
==========================================
  Files         348      348              
  Lines       10533    10594      +61     
==========================================
+ Hits         6649     6700      +51     
- Misses       3400     3409       +9     
- Partials      484      485       +1     
Files Coverage Δ
compiler/native/validate.go 97.29% <100.00%> (+0.37%) ⬆️
database/step/table.go 100.00% <ø> (ø)
mock/server/step.go 0.00% <ø> (ø)
api/build/restart.go 0.00% <0.00%> (ø)
api/build/plan.go 0.00% <0.00%> (ø)
api/build/create.go 0.00% <0.00%> (ø)
scm/github/repo.go 76.40% <84.00%> (+1.24%) ⬆️
api/build/compile_publish.go 0.00% <0.00%> (ø)

@ecrupper ecrupper marked this pull request as ready for review March 28, 2024 19:00
@ecrupper ecrupper requested a review from a team as a code owner March 28, 2024 19:00
@ecrupper ecrupper changed the title feat: step status reporting + fix pull request context overwrite bug feat!: step status reporting + fix pull request context overwrite bug Mar 28, 2024
api/build/compile_publish.go Outdated Show resolved Hide resolved
@@ -136,12 +141,25 @@ func validateSteps(s yaml.StepSlice) error {
continue
}

if s, ok := reportMap[step.ReportAs]; ok {
return fmt.Errorf("report_as to %s for step %s is already targeted by step %s", step.ReportAs, step.Name, s)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@plyr4
Copy link
Contributor

plyr4 commented Mar 28, 2024

Question I had: should a skipped step be a success?

in my opinion i dont think so, that might make required status checks auto pass when it never ran

Copy link
Contributor

@KellyMerrick KellyMerrick left a comment

Choose a reason for hiding this comment

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

lgtm

@ecrupper ecrupper merged commit 5706d0f into main Mar 29, 2024
13 of 16 checks passed
@ecrupper ecrupper deleted the feat/step-status-reporting branch March 29, 2024 14:12
@JordanSussman
Copy link
Collaborator

@ecrupper do you think it makes sense to open up another community issue for expanding this functionality to work with stages?

@ecrupper
Copy link
Contributor Author

@ecrupper do you think it makes sense to open up another community issue for expanding this functionality to work with stages?

Definitely. Also somewhat related would be go-vela/community#910 with the idea of a "status" for a stage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants