Skip to content

feature: add filtering of instructions on the exec command#210

Merged
GregoryEssertel merged 6 commits intomasterfrom
GE-add-filter-flag
Jun 28, 2021
Merged

feature: add filtering of instructions on the exec command#210
GregoryEssertel merged 6 commits intomasterfrom
GE-add-filter-flag

Conversation

@GregoryEssertel
Copy link
Copy Markdown
Contributor

@GregoryEssertel GregoryEssertel commented Jun 18, 2021

Summary:

  • Add a --exclude/-e and --include/-i flags on the exec command to filter instructions. The filters are being validated then forwarded to SCLE where the filtering is done: https://github.com/strateos/lab/pull/1527
  • Minor: use email address to specify who sent the payload.

Require: https://github.com/strateos/lab/pull/1526 to be merged in lab as well

Usage:

cat autoprotocol.json | \
transcriptic exec -a lab.core.strateos.com/haven/wctest -w wc4-mcx1 -e 1-9 -i 7 -e op:provision

This command will first filter out instruction from index 1 to 9 (-e 1-9) and the provisions (-e op:provision). Then add back the instruction at index 7 (-i 7).

The order of -i/-e don't matter, it is first removing all the -e, then adding back all the -i.

Filters

Possible filter:

  • single index: 123
  • range: 123-456
  • key:value in the instruction: op:provision, or x_human!:false. The ! means that if the key (x_human) is not in the instruction, then the filter matches. Otherwise, without the '!' a missing key means that there is no match.

Filter example:

  • { "op": "seal", "x_human": false } and { "op": "seal" } match x_human!:false
  • { "op": "seal" } does not matchx_human:true, nor x_human:false

Test plan

Add unit tests

Manual testing

internal_protocols/clients/sd2/current_protocols/growth_curve [master*]$ transcriptic preview GrowthCurve | \
transcriptic exec -a lab.core.strateos.com/haven/wctest -w wc4-mcx1 -e x_human:true
Sending request to http://13.52.223.111:9000/testRun
Success. View http://lab.core.strateos.com/haven/wctest/dashboard to see the scheduling outcome.

or

internal_protocols/clients/sd2/current_protocols/growth_curve [master*]$ transcriptic preview GrowthCurve | \
transcriptic exec -a lab.core.strateos.com/haven/wctest -w wc4-mcx1 -e op:provision
Sending request to http://13.52.223.111:9000/testRun
Success. View http://lab.core.strateos.com/haven/wctest/dashboard to see the scheduling outcome.

image

Bonus with the newest features not yet merged:

image

@matthewkuangga
Copy link
Copy Markdown

Works perfectly on local env. Include and exclude works just as intended. Great new feature that can go hand in hand with the new APR error message index feature

Copy link
Copy Markdown
Contributor

@EribertoLopez EribertoLopez left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown

@matthewkuangga matthewkuangga left a comment

Choose a reason for hiding this comment

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

LGTM, works perfectly when I tested

Comment thread transcriptic/commands.py
invalid_filters.add(arg)
elif re.match(r"\d+-\d+", arg):
[s, e] = [int(v) for v in arg.split("-")]
if s > e or number_of_instructions <= e:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should also check here that s is > 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, I think '\d+' should enforce that right?
With not 0?

Copy link
Copy Markdown
Contributor

@scottyantipa scottyantipa left a comment

Choose a reason for hiding this comment

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

This looks good. I think one nice addition would be to display the result of the filtering to the user, like:

Successfully submitted 10/13 instructions

To re-inforce to the user that their filter was applied as expected

@GregoryEssertel GregoryEssertel merged commit 431c54c into master Jun 28, 2021
@GregoryEssertel GregoryEssertel deleted the GE-add-filter-flag branch June 28, 2021 23:25
@GregoryEssertel
Copy link
Copy Markdown
Contributor Author

@scottyantipa I didn't see your comments 😬

I will address them in another PRs. At first I did the filtering client side, and I was giving this info. But then I move it server side, I will update the message return by SCLE with this information!

EribertoLopez pushed a commit that referenced this pull request Nov 9, 2021
## Summary:

 - Add a `--exclude/-e` and `--include/-i` flags on the exec command to filter instructions. The filters are being validated then forwarded to SCLE where the filtering is done: https://github.com/strateos/lab/pull/1527
 - Minor: use email address to specify who sent the payload.

Require: https://github.com/strateos/lab/pull/1526 to be merged in lab as well
### Usage:
```
cat autoprotocol.json | \
transcriptic exec -a lab.core.strateos.com/haven/wctest -w wc4-mcx1 -e 1-9 -i 7 -e op:provision
```
This command will first filter out instruction from index 1 to 9 (`-e 1-9`) and the provisions (`-e op:provision`). Then add back the instruction at index 7 (`-i 7`).

The order of `-i/-e` don't matter, it is first removing all the `-e`, then adding back all the `-i`.

### Filters

Possible filter:
 - single index: 123
 - range: 123-456
 - key:value in the instruction: `op:provision`, or `x_human!:false`. The ! means that if the key (x_human) is not in the instruction, then the filter matches. Otherwise, without the '!' a missing key means that there is no match.

Filter example:
  - { "op": "seal", "x_human": false } and { "op": "seal" } match `x_human!:false`
  - { "op": "seal" } does not match`x_human:true`, nor `x_human:false`

### Test plan
Add unit tests

#### Manual testing

```
internal_protocols/clients/sd2/current_protocols/growth_curve [master*]$ transcriptic preview GrowthCurve | \
transcriptic exec -a lab.core.strateos.com/haven/wctest -w wc4-mcx1 -e x_human:true
Sending request to http://13.52.223.111:9000/testRun
Success. View http://lab.core.strateos.com/haven/wctest/dashboard to see the scheduling outcome.
```
or
```
internal_protocols/clients/sd2/current_protocols/growth_curve [master*]$ transcriptic preview GrowthCurve | \
transcriptic exec -a lab.core.strateos.com/haven/wctest -w wc4-mcx1 -e op:provision
Sending request to http://13.52.223.111:9000/testRun
Success. View http://lab.core.strateos.com/haven/wctest/dashboard to see the scheduling outcome.
```
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