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

Add expression parser evaulator for build filter #2200

Merged

Conversation

praxist
Copy link

@praxist praxist commented Sep 9, 2017

continuation of #2171 (comment), using the expression library from https://github.com/drone/expr

server/rpc.go Outdated
@@ -89,12 +91,27 @@ func (s *RPC) Next(c context.Context, filter rpc.Filter) (*rpc.Pipeline, error)
}
}

var st *expr.Selector
var err error
if filter.Expr != "" {
Copy link
Author

Choose a reason for hiding this comment

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

Thoughts on moving this logic into the fn declaration? That would reduce the need for having the extra variable declarations. Or is catching a potential error from the selector initialization important?

Choose a reason for hiding this comment

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

yes, for caching, otherwise it would re-parse the sql string every time :)

.drone.yml Outdated
@@ -7,6 +7,7 @@ pipeline:
image: golang:1.8
commands:
- go get -u github.com/drone/drone-ui/dist
- go get -u github.com/drone/expr

Choose a reason for hiding this comment

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

we can vendor this repository, and then remove this line. Will add a comment with instructions shortly

@bradrydzewski
Copy link

ok, quick guide to vendoring ...

download the govendor tool

go get -u github.com/kardianos/govendor

and make sure you have a local copy of the expr library

go get -u github.com/drone/expr

and then you can run this command:

govendor add github.com/drone/expr

this will add the code to the vendor directory, which gets checked into the repository.

@bradrydzewski
Copy link

Thoughts on moving this logic into the fn declaration? That would reduce the need for having the extra variable declarations. Or is catching a potential error from the selector initialization important?

I'm guessing you were looking to clean up / simplify the code. That file is definitely getting a bit unruly after a lot of changes and years of evolution ...

If you wanted to clean things up a bit, you could break out the function like this:

func createFilterFunc(filter rpc.Filter) (queue.Filter, error) {
	var st *expr.Selector
 	var err error

 	if filter.Expr != "" {
 		st, err = expr.ParseString(filter.Expr)
 		if err != nil {
 			return nil, err
 		}
 	}

	return func(task *queue.Task) bool {
 		if st != nil {
 			match, _ := st.Eval(expr.NewRow(task.Labels))
 			return match
 		}
 
  		for k, v := range filter.Labels {
  			if task.Labels[k] != v {
  				return false
  			}
  		}
  		return true
	}, nil
}

You could then invoke this above function to get the filter, and pass to the poller:

fn, err := createFilterFunc(filter)
if err != nil {
	return nil, err
}
task, err := s.queue.Poll(c, fn)

I am fine either way, so I'll leave the decision up to you :)

@praxist praxist force-pushed the build_filter_expression_parser branch from 034afa8 to 1e9e2ec Compare September 9, 2017 02:36
Vendor github.com/drone/expr
Vendor github.com/drone/expr/parse
@praxist praxist force-pushed the build_filter_expression_parser branch from 1e9e2ec to 4c2ff78 Compare September 9, 2017 02:40
@praxist
Copy link
Author

praxist commented Sep 9, 2017

good recommendation!

@bradrydzewski bradrydzewski merged commit 6cdf907 into harness:master Sep 10, 2017
@ozbillwang
Copy link

ozbillwang commented Sep 11, 2017

Nice job, well done!

@praxist & @bradrydzewski

Could you provide some instructions on how to use this feature?

@bradrydzewski
Copy link

bradrydzewski commented Sep 11, 2017

I am planning on writing official docs, but in the mean time, here is a quick overview.

The first step is to define a query used to filter which builds the agent will accept. The query expression is similar to sql. For example:

platform = 'linux/amd64' AND repo GLOB octocat/*

There are two fields available by default:

  • repo is the repository name
  • platform is the target platform for the build

The user can define additional fields in the labels section of the .drone.yml file. These fields can then be evaluated by the query expression:

pipeline:
  build:
    image: golang
    commands: go test

label:
  - ram=32
  - cpu=16

For example:

platform = 'linux/amd64' AND ram >= 32 AND cpu >= 16

The query expression string is passed to the agent using the DRONE_FILTER environment variable. The agent will only process builds when the expression evaluates to true. In the previous example, the agent would only process builds for linux, with user-defined labels specifying 32 GB ram or higher and 16 cores or higher.

@yoanisgil
Copy link

@bradrydzewski is it possible to define labels at each step so individual steps are executed by a given agent? It does not look like is possible right now but I just wanted to confirm.

@bradrydzewski
Copy link

It does not look like is possible right now but I just wanted to confirm

It is not possible, but perhaps this is something that would be possible when we launch the .drone.js file, which will have more advanced scripting and conditional capabilities.

@yoanisgil
Copy link

Quick question though: this new upcoming functionality in .drone.js, is something that can be tested? Like we're in the process of choosing our CI tool and I personally like Drone and features such as this one would really help in the decision process (as well as the monorepo one ;)).

@bradrydzewski
Copy link

There should be a release candidate for 0.9 end of month, which will include mono-repo support. The .drone.js will probably be part of 0.10. So will be a while.

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.

None yet

4 participants