-
Notifications
You must be signed in to change notification settings - Fork 66
Templating, lower_case variables and files #361
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
Conversation
Awesome! I like the new lower-case names. (Do we even need the longer-form names?) |
Well, we don't need the longer-form names, but as I said in the sync meeting: the lower-case names have a bunch of drawbacks, since they're super-dynamically-typed (everything is a I think for a v0.1 of templated-campaign specs that's fine as long as we flag it as experimental |
f8fde2d
to
ff92b45
Compare
Agreed. I also prefer the lowercase, un- I think the design here is solid, and we should move on to reviewing and landing this once you're happy with the implementation. Nice job! |
Question: does |
So in your scenario, the 3.21 branch would be at the head of the default branch, and then |
Yep - where the So I imagine templating in the changeset template would be, if repo is deploy-sourcegraph, set base branch to |
I see, thanks for elaborating! Currently we don't have support for creating the base branch. Setting the base branch to an existing branch, however, is a planned feature (also specifying per repo is coming). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 🌟 Very cool feature and makes using campaign specs so much simpler
Here's another example campaign that can be run with this prototype (adding this here so I have a link to share): # Step 1: build the `ruplacer` Docker image
# $ cat Dockerfile
# FROM rust
# RUN cargo install ruplacer
#
# $ docker build -t ruplacer .
#
#
# (Why use ruplacer? Because it supports `--subvert` which allows us to
# replace `camelCase`, `snake_case`, `ThisCase`, `nocase`.)
#
# Step 2: use the src-cli prototype with templating support
#
# $ cd src-cli && git fetch && git checkout mrnugget/templates-and-files
# $ go build ./cmd/src -o ~/bin/src
#
# Step 3: update the `repositoriesMatchingQuery` to include or exclude file types.
#
# Step 4: run this campaign
#
# $ src campaign preview -f update-language.campaign.yaml
name: update-language
description: This campaign changes occurrences of whitelist & blacklist to allowlist & denylist.
on:
- repositoriesMatchingQuery: whitelist OR blacklist -file:scss$ -file:html$ repo:github.com/sourcegraph/sourcegraph
steps:
- run: |
cat /tmp/search-results | while read file;
do
ruplacer --subvert whitelist allowlist --go ${file} || echo "nothing to replace";
ruplacer --subvert blacklist denylist --go ${file} || echo "nothing to replace";
done
container: ruplacer
files:
/tmp/search-results: ${{ join repository.search_result_paths "\n" }}
# Describe the changeset (e.g., GitHub pull request) you want for each repository.
changesetTemplate:
title: Replace usage of whitelist & blacklist with allowlist & denylist
body: This replaces usages
branch: campaigns/allowlist-denylist
commit:
message: Use allowlist/denylist in wording
published: false |
Adds basic campaigns integration into our release process by using the `importChangesets` feature on pull requests that our release tooling already generates. `release:publish` now generates a new campaign - we do not use src-cli to generate the changes right now, since it is not currently possible without per-repository steps (though it might be enabled by sourcegraph/src-cli#361 in the future) `release:add-to-campaign` adds an existing change to release-tracking campaign, for example to track a blog post pull request.
That's what I meant with requesting the review 😄 But I should've been clearer: I think the implementation as it is now is fine to be merged as an experimental feature, which we will modify and tweak in the future. I checked that this branch here works with I'd love to merge this as soon as possible. |
Sorry, I think I somehow thought this was still in draft when I looked at it Monday. Real review coming shortly! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited to see this land!
This fixes #382 by fixing a regression that was introduced by #361. When the search queries defined in a campaign spec yielded a repository match result and file match results in the same repository, in that order, the code would crash because of an uninitialized map. The code here fixes it by always initializing the map. I also added a test that reproduces the behaviour and documents how we turn multiple search results into a list of repositories.
This fixes #382 by fixing a regression that was introduced by #361. When the search queries defined in a campaign spec yielded a repository match result and file match results in the same repository, in that order, the code would crash because of an uninitialized map. The code here fixes it by always initializing the map. I also added a test that reproduces the behaviour and documents how we turn multiple search results into a list of repositories.
* Add support for templated campaign specs * Make search results accessible in campaign spec templates * Remove debug log * Remove shortcuts in templates for now * Report rendered steps * Remove unused parameter * Fix build * Add lower-case aliases and stdout/stderr to templates * Add test to check that empty context works * Nil-safe templates * Mount templated files into src-cli * WIP * Undo local file mounting * Allow template vars in step env * Clean up code a little bit * Add changelog entry * Fix test * Omit step.Files from CampaignSpec if empty for backwards-compatibility * Update CHANGELOG.md Co-authored-by: Chris Pine <[email protected]> * Clean up temp file code * Return slices of strings instead of whitespace-separated string * Incorporate Adam's feedback Co-authored-by: Chris Pine <[email protected]>
This fixes #382 by fixing a regression that was introduced by #361. When the search queries defined in a campaign spec yielded a repository match result and file match results in the same repository, in that order, the code would crash because of an uninitialized map. The code here fixes it by always initializing the map. I also added a test that reproduces the behaviour and documents how we turn multiple search results into a list of repositories.
This builds on top of #334 and adds templating to campaign spec including lower-case variable names and access to STDOUT/STDERR.
Updated campaign spec from previous prototype:
Details
${{ .Repository.SearchResultPaths }}
and${{ repository.search_result_paths }}
${{ .Repository.Name }}
and${{ repository.name }}
and the other fields and methods on*graphql.Repository
${{ .PreviousStep.ModifiedFiles }}
and${{ previous_step.modified_files }}
${{ .PreviousStep.AddedFiles }}
and${{ previous_step.added_files }}
${{ .PreviousStep.DeletedFiles }}
and${{ previous_step.deleted_files }}
${{ .PreviousStep.Stdout }}
and${{ previous_step.stdout }}
${{ .PreviousStep.Stderr }}
and${{ previous_step.stderr}}
${{ join <list> <sep> }}
, example:${{ join repository.search_result_paths "\n" }}
${{ split <string> <sep> }}
, example:${{ split repository.name "/" }}
TODOs
Support mounting local filesErik and I decided against this, since that would make server-side execution harder.env
too