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

refactor(ruleset)!: improve match code efficiency and properly handle status #1255

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ecrupper
Copy link
Contributor

@ecrupper ecrupper commented Feb 17, 2025

steps:
  - name: test
    image: alpine:latest
    ruleset:
      event: pull_request
      branch: [ hotfix-* ]
      operator: or
    commands:
      - echo blah

The above does not purge. It only gets skipped when the worker handles skipping. The compiler should be corrected here and return FALSE when the above is calculated with rule data == push on main for example

Further, I refactored the match code to be more efficient. I removed the unnecessary Parallel key, which will be addressed in a follow-up worker PR, which introduces stage status.

Added some bench mark testing which is just there for fun right now, but I used it to confirm the improvements by comparing these changes to main. Improvement of about half a second, which definitely adds up over parsing thousands of rulesets.

Another thing I added was separation of if and unless operators/matchers if defined. Otherwise, they'll inherit from the Ruleset like usual (non-breaking).

@ecrupper ecrupper requested a review from a team as a code owner February 17, 2025 21:08
Copy link

codecov bot commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.61%. Comparing base (3da62c5) to head (e61458c).

❌ Your project check has failed because the head coverage (56.61%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1255      +/-   ##
==========================================
+ Coverage   56.55%   56.61%   +0.06%     
==========================================
  Files         630      630              
  Lines       35710    35663      -47     
==========================================
- Hits        20195    20190       -5     
+ Misses      14828    14800      -28     
+ Partials      687      673      -14     
Files with missing lines Coverage Δ
compiler/native/compile.go 70.37% <100.00%> (+0.06%) ⬆️
compiler/native/native.go 80.74% <ø> (ø)
compiler/registry/github/github.go 100.00% <ø> (ø)
compiler/types/pipeline/container.go 83.67% <100.00%> (+0.75%) ⬆️
compiler/types/pipeline/ruleset.go 100.00% <100.00%> (+21.71%) ⬆️
compiler/types/pipeline/secret.go 94.59% <100.00%> (ø)
compiler/types/pipeline/stage.go 96.34% <100.00%> (ø)
compiler/types/yaml/buildkite/ruleset.go 83.81% <100.00%> (+1.99%) ⬆️
compiler/types/yaml/yaml/ruleset.go 66.36% <100.00%> (+3.17%) ⬆️
database/build/build.go 51.72% <ø> (ø)
... and 15 more
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

plyr4
plyr4 previously approved these changes Feb 17, 2025
KellyMerrick
KellyMerrick previously approved these changes Feb 17, 2025
@ecrupper ecrupper changed the title fix(ruleset): proper match for status with Or operator refactor(ruleset)!: improve match code efficiency and properly handle status Feb 18, 2025
timhuynh94
timhuynh94 previously approved these changes Mar 12, 2025
KellyMerrick
KellyMerrick previously approved these changes Mar 12, 2025
@ecrupper ecrupper dismissed stale reviews from KellyMerrick and timhuynh94 via e61458c March 20, 2025 13:54
@@ -30,7 +30,7 @@ func (c *client) Equal(other *client) bool {
// New returns a Registry implementation that integrates
// with GitHub or a GitHub Enterprise instance.
//
//nolint:revive // ignore returning unexported client

func New(ctx context.Context, address, token string) (*client, error) {

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unexported-return: exported func New returns unexported type *github.client, which can be annoying to use (revive)

@@ -42,7 +42,7 @@ type (

// New creates and returns a Vela service for integrating with settings in the database.
//
//nolint:revive // ignore returning unexported engine

func New(opts ...EngineOpt) (*engine, error) {

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unexported-return: exported func New returns unexported type *settings.engine, which can be annoying to use (revive)

@@ -40,7 +40,7 @@ type (

// New creates and returns a Vela service for integrating with steps in the database.
//
//nolint:revive // ignore returning unexported engine

func New(opts ...EngineOpt) (*engine, error) {

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unexported-return: exported func New returns unexported type *step.engine, which can be annoying to use (revive)

@@ -40,7 +40,7 @@ type (

// New creates and returns a Vela service for integrating with services in the database.
//
//nolint:revive // ignore returning unexported engine

func New(opts ...EngineOpt) (*engine, error) {

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unexported-return: exported func New returns unexported type *service.engine, which can be annoying to use (revive)

@@ -42,7 +42,7 @@ type (

// New creates and returns a Vela service for integrating with repos in the database.
//
//nolint:revive // ignore returning unexported engine

func New(opts ...EngineOpt) (*engine, error) {

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unexported-return: exported func New returns unexported type *repo.engine, which can be annoying to use (revive)

@@ -53,7 +53,7 @@ type (

// New creates and returns a Vela service for integrating with dashboards in the database.
//
//nolint:revive // ignore returning unexported engine

func New(opts ...EngineOpt) (*engine, error) {

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unexported-return: exported func New returns unexported type *dashboard.engine, which can be annoying to use (revive)

@@ -59,7 +59,7 @@ type (

// New returns a Secret implementation that integrates with a Vault secrets engine.
//
//nolint:revive // ignore returning unexported client

func New(opts ...ClientOpt) (*client, error) {

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unexported-return: exported func New returns unexported type *vault.client, which can be annoying to use (revive)

@@ -74,7 +74,7 @@ type client struct {
// New returns a SCM implementation that integrates with
// a GitHub or a GitHub Enterprise instance.
//
//nolint:revive // ignore returning unexported client

func New(ctx context.Context, opts ...ClientOpt) (*client, error) {

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unexported-return: exported func New returns unexported type *github.client, which can be annoying to use (revive)

@@ -238,7 +238,7 @@ func (c *client) ValidateGitHubApp(ctx context.Context) error {
//
// This function is intended for running tests only.
//
//nolint:revive // ignore returning unexported client

func NewTest(urls ...string) (*client, error) {

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unexported-return: exported func NewTest returns unexported type *github.client, which can be annoying to use (revive)

@@ -55,7 +55,7 @@ type client struct {

// FromCLIContext returns a Pipeline implementation that integrates with the supported registries.
//
//nolint:revive // ignore returning unexported client

func FromCLIContext(ctx *cli.Context) (*client, error) {

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unexported-return: exported func FromCLIContext returns unexported type *native.client, which can be annoying to use (revive)

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

Successfully merging this pull request may close these issues.

4 participants