Skip to content

Conversation

@ecrupper
Copy link
Contributor

@ecrupper ecrupper commented Oct 7, 2022

Closes go-vela/community#547

A couple things to note about this PR:

  1. It does not account for runtime rules, such as continue and status. The reason being: templates are compiled and merged into the parent pipeline, and there is no way of referring to a template-level ruleset once that has happened.

  2. This PR struck me as the only way to accommodate rulesets at a template level without re-hauling a bit. Another idea would be to create a template field for each pipeline.Build which carries the slice of steps (or perhaps references to them) as well as the ruleset. In other words: a lot of work.

That said, I recognize this solution is a bit hacky, and if we'd rather come to a conclusion on whether or not we want this feature in the first place (and if so, probably work on a proposal to establish a design), that would be a viable outcome for this PR! Let me know your thoughts!

@ecrupper ecrupper self-assigned this Oct 7, 2022
@ecrupper ecrupper requested a review from a team as a code owner October 7, 2022 23:22
@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Merging #720 (98061bd) into main (931e938) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #720      +/-   ##
==========================================
+ Coverage   58.09%   58.12%   +0.03%     
==========================================
  Files         255      255              
  Lines       15880    15892      +12     
==========================================
+ Hits         9226     9238      +12     
  Misses       6249     6249              
  Partials      405      405              
Impacted Files Coverage Δ
compiler/native/compile.go 62.24% <100.00%> (ø)
compiler/native/expand.go 72.41% <100.00%> (+1.73%) ⬆️

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

A little hacky, yes. But I think that's ok here. Let's get the functionality and then figure out if we need a more extensive refactor.

Copy link
Contributor

@jbrockopp jbrockopp 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 4a275f6 into main Apr 12, 2023
@ecrupper ecrupper deleted the template-ruleset branch April 12, 2023 21:03
Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

👍🏼

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.

Ruleset to control the use of templates

6 participants