Skip to content

Support arg+ syntax for argument capture#108

Closed
johnrengelman wants to merge 1 commit intotarget:masterfrom
johnrengelman:varargs
Closed

Support arg+ syntax for argument capture#108
johnrengelman wants to merge 1 commit intotarget:masterfrom
johnrengelman:varargs

Conversation

@johnrengelman
Copy link

Proposed change

Allow a rule to capture the remaining arguments by adding a + to the argument name in the rule.
Variable argument capture can only be activated on a single argument at the end of the argument list. It cannot be combined with optional arguments.

Types of changes

What types of changes is this pull request introducing to flottbot? Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

You can fill this out after creating your PR. Put an x in the boxes that apply

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 229

  • 0 of 26 (0.0%) changed or added relevant lines in 1 file are covered.
  • 431 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-21.9%) to 17.893%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core/matcher.go 0 26 0.0%
Files with Coverage Reduction New Missed Lines %
core/prommetric.go 2 0.0%
core/configure.go 113 0.0%
core/matcher.go 316 0.0%
Totals Coverage Status
Change from base Build 227: -21.9%
Covered Lines: 360
Relevant Lines: 2012

💛 - Coveralls

@wass3r wass3r changed the title Upport arg+ syntax for argument capture Support arg+ syntax for argument capture Sep 10, 2019
Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

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

@johnrengelman thanks for the contribution! Just one quick fix. I have tested otherwise and seems good to me. If you're up to it, would love to see this mentioned in docs. I created an issue for it at target/flottbot-docs#34.

message.Output = msg
return false
}
if strings.HasSuffix(rule.Args[len(rule.Args)-1], "+") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super quick fix here. This trips up when length of rule.Args is zero. A fix here should get us past the tests.

@wass3r
Copy link
Collaborator

wass3r commented Nov 5, 2019

Thanks @johnrengelman for the idea and majority of the work. We finished it out in #116

@wass3r wass3r closed this Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants