Skip to content

PROTOTYPE: Magic files to access search results/modified files #357

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

Conversation

mrnugget
Copy link
Contributor

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 another prototype, next to #334, to solve the same thing: allowing step.run commands to access specific files.

What this prototype does is to create magic files inside the container. The files contain lists of files, one filename per line. By using standard shell scripting, the run commands can then access these files.

Example campaign spec:

name: a-magic-file-appeared
description: Updated description Hello World to READMEs

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

steps:
  # The two magic files right now are:
  #
  #   /src/search-results                contains search results in current repository, one per line
  #   /src/previous-step/modified-files  contains files modified by previous step, one per line
  #
  # How do you make use of them?
  #
  # Option 1) Use a loop:
  - run: cat /src/search-results | while read result; do xargs comby -in-place 'fmt.Sprintf("%d", :[v])' 'strconv.Itoa(:[v])' "${result}"; done
    container: comby/comby
  # Option 2) Use cat, which Unix-snobs say is useless here:
  - run: cat /src/search-results | xargs comby -in-place 'fmt.Sprintf("%d", :[v])' 'strconv.Itoa(:[v])' 
    container: comby/comby
  # Option 3) The Unix-snobs-approved variant: use the file as STDIN
  - run: </src/search-results xargs comby -in-place 'fmt.Sprintf("%d", :[v])' 'strconv.Itoa(:[v])' 
    container: comby/comby
  # We just ran comby three times. Now let's use `goimports` to format the modified files:
  - run: cat /src/previous-step/modified-files | xargs goimports -w
    container: unibeautify/goimports

# [...]

Screenshot

screenshot_2020-10-19_14 55 05@2x

Details

This branch here builds on top of #334. But instead of using templating, it simply makes the data available in temp files that are then mounted into the container.

Drawbacks

  • shell scripting required. It's not super advanced, but it also might not be as straightforward as the templating approach.

@mrnugget mrnugget requested a review from a team October 19, 2020 13:01
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

Code-wise this looks good.
I also think there are valid use-cases for such a pass-in to the container, for example the case where argument lists would become too long for bash to process.
However, I think it is heavier to use and (at least from my perspective) a little harder to understand. Probably mostly because I personally never saw such a pattern before, at least not such an implicit pattern.
I was reminded of kubernetes secrets/configmaps when thinking about this and I thought: Why not make a hybrid of this file-based stuff and of the variables syntax, while making the use of files a little more explicit as well.

Example:

steps:
  - image: golang:1.14
    command: go fmt ${{ search_results }} # I assume there are only a few, so I pass it in directly, for the sake of simplicity.
  - image: node:14
    command: </modified-files xargs npm run prettier -c /prettier/conf.js
    mount:
     # Allows for arbitrary files that are needed.
      /prettier.conf.js: |
         useCurly: true
         other super important config value: oh yeah
      # As well as files that contain the content of a yaml variable
      /modified-files: ${{ join modified_files "\n" }} # hell that's a lot of files, need to put them in a file

That way

  • it is explicit where files are mounted that hold the variables content
  • is similar to how kubernetes would do it
  • Also covers the case where the user needs to pass-in a config file or something
  • Allows to use both variables AND files

I haven't really thought this through, but I could imagine it is similarly easy to implement and would take the best of the two prototypes.

@chrispine
Copy link
Contributor

Cool idea! Like Erik, I wonder if this is just a bit less user-friendly than templates.

Here's a (possibly terrible) idea... what if this:
command: go fmt ${{ search_results }}
just ran the command once for each search result?

@eseliger
Copy link
Member

Here's a (possibly terrible) idea... what if this:
command: go fmt ${{ search_results }}
just ran the command once for each search result?

That can be done using echo "${{ search_results }}" | xargs go fmt, so it's possible, requires just using one of the thousands of tools of the bash swiss army knife tools.
However, the opposite won't be so easy if it would always invoke the command multiple times. There are tools that build up internal state, like many linters do for example. They usually need to consume the entire codebase and build the AST internally, before they can do their magic on the file. Calling the linter N times would multiply that warmup by N times :/

@mrnugget
Copy link
Contributor Author

Here's a (possibly terrible) idea... what if this:

Yeah, I thought about something similar, but what I ended up thinking about was more along the lines of

for_each_search_result: go fmt ${{ search result }}

But I think we should provide more primitives, because those will allow user to do that and more.

I'll try to see if I can implement Erik's idea today.

@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