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: add new proposal for dynamic ruleset #1026

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JordanSussman
Copy link
Collaborator

@JordanSussman JordanSussman commented Dec 27, 2024

Adding a new proposal to allow dynamic ruleset without needing to invoke templates.

@JordanSussman JordanSussman requested a review from a team as a code owner December 27, 2024 16:27
Copy link
Contributor

@ecrupper ecrupper left a comment

Choose a reason for hiding this comment

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

I love this idea. It's a lot cleaner than the Go templating. I do think it should be nested inside ruleset though. It'll be easier to implement IMO. But that is just my opinion 😅


## Questions

1. Should we name the new key `evaluation` or choose a different name?
Copy link
Contributor

Choose a reason for hiding this comment

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

I like evaluation personally.

Copy link
Member

Choose a reason for hiding this comment

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

i know if is off the table and when is confusing in relationship to if, so evaluation seems fine if a bit lengthy. i don't feel strongly about this, but eval could be an alternative.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like eval instead of evaluation.

## Questions

1. Should we name the new key `evaluation` or choose a different name?
2. Should the new key be nested under `ruleset` or be at the same level?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I like nested. It's almost like a custom rule of sorts.


1. Should we name the new key `evaluation` or choose a different name?
2. Should the new key be nested under `ruleset` or be at the same level?
3. Should it be possible to use both the existing `ruleset` and the new `evaluation` feature simultaneously? If so, should both need to return true for the step to execute?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think it should be possible to use both. Something like

ruleset:
  event: push
  branch: main
  evaluation: "hasPrefix(VELA_BUILD_AUTHOR, 'Jordan')"

Of course you could add the other two rules into the evaluation, but that feels a bit messy. So I like being able to declare them separately as they normally are.

@JordanSussman
Copy link
Collaborator Author

I do think it should be nested inside ruleset though. It'll be easier to implement IMO. But that is just my opinion

Glad we are in agreement. The examples show it nested under the ruleset. I left it as an open question in case anyone thinks otherwise.


- name: new-and-old
ruleset:
evaluation: "VELA_BUILD_AUTHOR == 'JordanSussman'"
Copy link
Member

Choose a reason for hiding this comment

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

i think these are just pseudo examples. how would we actually access and make available the build env vars?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how would we actually access and make available the build env vars

The example code in the expr README demonstrates how to handle the underlying code for injecting env vars. Additionally, I created a simple Go playground to showcase a more Vela-specific implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wass3rw3rk, just to clarify, are you saying that the built-in Vela environment variables aren't available when the rulesets are evaluated, compared to how we would write the underlying expr code to inject them?

Copy link
Member

Choose a reason for hiding this comment

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

the code example is what i was looking for - ty! i thought you had to do more to reference a variable 👍🏼

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still need to spend some time testing it out to confirm the environments are correctly populated, but I have the initial server code changes within go-vela/server#1233.

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.

looks great overall. this will be a nice addition.

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

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