Skip to content

PROTOTYPE: Add support for templated campaign specs #334

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

Closed
wants to merge 7 commits into from

Conversation

mrnugget
Copy link
Contributor

@mrnugget mrnugget commented Oct 1, 2020

NOTE FOR REVIEWERS: This is a prototype and should be treated as such, which means: let's review idea/design/feasibility instead of concrete implementation details (but if you do have concrete feedback on the implementation, feel free to leave it as such).


This is a prototype to add templating to the steps definitions in a campaign spec YAML file.

It's similar to the templating allowed in GitHub Actions.

Here is an excerpt from a campaign spec that uses templating. It runs comby only over the search results found in the repository by the repositoriesMatchingQuery (that's the ${{ .Repository.SearchResultPaths }} in the spec) and then runs goimports only over the files modified by comby (the ${{ .PreviousStep.ModifiedFiles }}):

name: templated-campaign
description: Use templates

on:
  - repositoriesMatchingQuery: lang:go fmt.Sprintf("%d", :[v]) patterntype:structural -file:vendor count:10

steps:
  - run: comby -in-place 'fmt.Sprintf("%d", :[v])' 'strconv.Itoa(:[v])' ${{ .Repository.SearchResultPaths }}
    container: comby/comby
  - run: goimports -w ${{ .PreviousStep.ModifiedFiles }}
    container: unibeautify/goimports

changesetTemplate:
  # [... changesetTemplate is unchanged ...]

Screenshot

image

Details

This is a tested prototype that works. Executing campaigns like this is much faster because comby only has to look at and touch the files it needs to modify, same goes for goimports.

What else is available in the templates? This is the definition of StepContext, which is passed into the step.run string before executing a step.

That means a user can access:

Drawbacks

  • there are no shortened aliases available as variables (${{ search_result_paths }} for example). Why? Because I couldn't come up with a consistent naming scheme that allows us to have a hierarchy too (like previous_step.modified_files etc.). That doesn't mean we shouldn't do it though, since I don't like the .Foobar syntax of Go's templating system.
    • Edit: I managed to get lower_case names + field access working (see here), which means we could use that instead.
  • it might not be clear to the user that they have access to file paths since they're defining *repositories*MatchingQuery in their campaign spec. There might be a better way to define this.
  • with a large number of files and search results the rendered commands might become very long and thus hard to debug

@mrnugget mrnugget changed the title Add support for templated campaign specs PROTOTYPE: Add support for templated campaign specs Oct 1, 2020
@chrispine
Copy link
Contributor

This is really cool! I love it

@mrnugget mrnugget force-pushed the mrnugget/templated-specs branch from d38c9ac to 4022b92 Compare October 19, 2020 11:59
@mrnugget mrnugget marked this pull request as ready for review October 19, 2020 12:05
@mrnugget mrnugget requested a review from a team October 19, 2020 12:06
@chrispine
Copy link
Contributor

I love how this is coming together! Thoughts on the drawbacks you mentioned:

  • It's okay if our first release of this is only long-form names, as long as those don't break later. We don't have to figure out the perfect short-form naming right away.
  • I feel like Repository.SearchResultPaths is pretty clear that it's paths, not repos, even though the paths come out of repositoriesMatchingQuery. (IMHO)
  • This could be an issue. Plus, don't shells often have limits on how long a command can be? Can each item be run as a separate command?

@LawnGnome
Copy link
Contributor

LawnGnome commented Oct 20, 2020

  • there are no shortened aliases available as variables (${{ search_result_paths }} for example). Why? Because I couldn't come up with a consistent naming scheme that allows us to have a hierarchy too (like previous_step.modified_files etc.). That doesn't mean we shouldn't do it though, since I don't like the .Foobar syntax of Go's templating system.

    • Edit: I managed to get lower_case names + field access working (see here), which means we could use that instead.

Are there any drawbacks to how that's implemented? If it supports the full range of functionality the default Go template syntax does, I feel like we should just make that the standard and be done with it, since I agree it's more readable.

  • with a large number of files and search results the rendered commands might become very long and thus hard to debug

I think our limit here is going to be 131,072 bytes on Linux, regardless of configuration, because we'll hit MAX_ARG_STRLEN when starting the container.

So, yeah, that's not a lot when you're dealing with a big vendored node_modules.

The ideal solution to me feels like it would be to do something sort of xargs-y: detect if we're going to go over MAX_ARG_STRLEN or ARG_MAX as configured, and then run the Docker container repeatedly with different subsets of the template expansion. Unfortunately, that probably opens its own can of worms in terms of what happens if the different runs of the same step somehow interact badly.

For now, we could probably just detect this situation independent of template expansion when running the campaign steps, and warn the user.

Edit: Yeah, OK, I read this before getting to #357 in my notifications. Take it for what it's worth.

@mrnugget
Copy link
Contributor Author

Are there any drawbacks to how that's implemented? If it supports the full range of functionality the default Go template syntax does, I feel like we should just make that the standard and be done with it, since I agree it's more readable.

The most obvious drawback is that we need to "manually" add aliases for fields: https://gist.github.com/mrnugget/774812fe534fb019a80ad9be2c364a57#file-lower_case_var_hierarchy-go-L36-L45

Reasonably sure, though, that we could somehow automate this by — hear me out — converting to JSON and then back to map[string]interface{}

@mrnugget
Copy link
Contributor Author

Closing in favor of #361.

@mrnugget mrnugget closed this Oct 26, 2020
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.

3 participants