-
Notifications
You must be signed in to change notification settings - Fork 43
Stab at using user_agent: [ server, local ] type of matching for sele… #5
base: master
Are you sure you want to change the base?
Conversation
…cting where a step should be able to run
bradrydzewski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome! just some minor suggestions, but great work :)
pipeline/frontend/yaml/constraint.go
Outdated
| Status Constraint | ||
| Matrix ConstraintMap | ||
| Local types.BoolTrue | ||
| User_agent Constraint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do this:
UserAgent Constraint `yaml:"user_agent"`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, didn't know. but done :)
pipeline/frontend/yaml/constraint.go
Outdated
| // Match returns true if all constraints match the given input. If a single | ||
| // constraint fails a false value is returned. | ||
| func (c *Constraints) Match(metadata frontend.Metadata) bool { | ||
| func (c *Constraints) Match(metadata frontend.Metadata, local bool) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe instead, add useragent to the metadata?
https://github.com/cncd/pipeline/blob/master/pipeline/frontend/metadata.go#L85
|
@bradrydzewski let me know if that works for you. I'm not exactly sure where to set the value for the UserAgent in the metadata but whats there now seems to work :) |
|
thanks! let's get some more input on this, but so far looks great |
tonglil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local and server user-agents can be a little confusing. What sets the container's constraints (to ensure that by default they require either server or local)?
pipeline/frontend/yaml/constraint.go
Outdated
| Status Constraint | ||
| Matrix ConstraintMap | ||
| Local types.BoolTrue | ||
| UserAgent Constraint `yaml:"user_agent"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gofmt indent
The client and server would each have a different user-agent string that it would pass to the build for evaluation at runtime. If the yaml does not define any user-agent conditions, it would run for all user agents. Definitely open to other ideas / less confusing names |
|
Would remote/local be better then server/local? Although remote is already used for the source of the repos. :/ |
|
What about master/agent? That is the meaning that I am getting by reading local (master) and server (Drone agents). |
|
Master agent make me more think of master being the drone server and agent being the build agent. But I could see local/agent as something that may make sense? |
|
@bradrydzewski was just going to check back in on this if we wanted to move forward with it or something else :) |
|
actually, the plan is to remove for example: This could be provided with the following compiler options: This would add or remove these steps at runtime. |
|
Ah, I like that better, less clutter in the .drone.yml but how does it handle something that should only run locally but not on the build agent? |
|
Sorry for sitting on this for so long, but sometimes sitting on things gives me time to gather my thoughts and talk with other users about their use cases. After careful consideration I think we need both Sorry again for the delay 😄 |
# Conflicts: # pipeline/frontend/yaml/constraint.go # pipeline/frontend/yaml/constraint_test.go
|
@bradrydzewski Should I try to keep this PR up to date or do you have other plans for it :) |
@bradrydzewski Here's a working version of
user_agent: [ server, local ]
selection for where the step should run. This should allow to select both local and server or either or. My implementation feels a little hackish so hopefully but you might have an idea on how it would better work.