jpoehnelt/secrets-sync-action

[Discussion] Ideas for v2

jcbhmr opened this issue · 8 comments

jcbhmr commented

👋 Hello @jpoehnelt ! It's me, the person who wanted to contribute some ideas that I had experimented on my own back to this repo (since it's the most popular). Here's some ideas for a @v2 release. These are some pretty drastic changes, and are very focused on improving the usability of this action and making it conform with existing conventions.

Here's a general overview of what I'd like to change. I know this is a lot but hear me out. I'll try to provide a rationale for each change:

  1. Use Deno or https://github.com/JasonEtco/build-and-tag-action. This gets rid of the pesky dist/ folder in source control which is a bit complex to make sure is in sync with code changes. Heck, if it's simple enough, you could even use Bash! (remember Bash is also on Windows runners via git bash) or even Python if you want! The key I think is to scale complexity of managing the project with the complexity of the code in the project. In this case, npm + dist folder is a bunch of extra stuff
  2. Use kebab-case inputs since this is what all the official actions use https://github.com/actions. It's also what most other user-made actions use and appears to be "the convention"
  3. Add aliases for repository repositories and other plurals. This greatly reduces the potential for typos and user frustration.
  4. Use GitHub Search syntax instead of regex to match multiple repos dynamically. This is more familiar to github users and very easy to test which repos it will apply to! Just use https://github.com/search!
  5. Remove (some) linting (config). This is more of a "this is excessive" opinion, but I think that in particular all the eslint and such could be replaced with deno lint or npx xo or something. Even then, there's enough safety with tsc that I don't think this project has reached "big status" to need such fancy linting stuff.
  6. Use more practical e2e tests instead of unit tests. Sure, ts tests are ok, but they don't compare to uses: ./ to actually run your action in CI. I find that a dry-run: flag plus a "real world" test or two works pretty good.
  7. Add a comparison blurb to the readme (after everything is said and done) about how this project compares to other secrets sync actions https://github.com/marketplace?type=actions&query=secrets+sync+
  8. Improve the "feel" of the readme. See https://github.com/Andrew-Chen-Wang/github-wiki-action for what I consider to be "good feel"
  9. Use explicit secrets instead of regex secrets. Sure, it's a tad harder to say "all secrets", but it really really helps for keeping which secrets sync where very explicit. And given that you're usually syncing only 1-5 secrets, this isn't much of a hassle that regex confusion is worth it imo.
  10. Use https://github.com/actions/publish-action to atuo-create v2 major tags from a release like v2.0.0
  11. Also support https://docs.github.com/en/actions/learn-github-actions/variables

You can kinda see my influence on this action that I've helped out with before: https://github.com/Andrew-Chen-Wang/github-wiki-action 👈 deliberately low tooling, as simple as possible.

Here's some ideas for interfaces of what I think could be cool:

This uses the mode: input to reuse the secrets and vars inputs for either set/delete operations

- uses: jpoehnelt/secrets-sync-action@v2
  with:
    token: ${{ secrets.MY_SECRETS_TOKEN }}
    repositories: |
      octocat/awesome-project
      octocat/my-library
      octocat/hello-world
    mode: set
    secrets: |
      NPM_TOKEN=${{ secrets.NPM_TOKEN }}
      VERCEL_TOKEN=${{ secrets.VERCEL_TOKEN }}
    vars: |
      NODE_VERSION=${{ vars.NODE_VERSION }}
      MESSAGE=hello
- uses: jpoehnelt/secrets-sync-action@v2
  with:
    token: ${{ secrets.MY_SECRETS_TOKEN }}
    repositories: |
      octocat/awesome-project
      octocat/my-library
      octocat/hello-world
    mode: delete
    secrets: |
      NPM_TOKEN=${{ secrets.NPM_TOKEN }}
      VERCEL_TOKEN=${{ secrets.VERCEL_TOKEN }}
    vars: |
      NODE_VERSION=${{ vars.NODE_VERSION }}
      MESSAGE=hello

As an alternative, you could make each one a separate input to be more explicit:

- uses: jpoehnelt/secrets-sync-action@v2
  with:
    token: ${{ secrets.MY_SECRETS_TOKEN }}
    repositories: |
      octocat/awesome-project
      octocat/my-library
      octocat/hello-world
    set-secrets: |
      NPM_TOKEN=${{ secrets.NPM_TOKEN }}
      VERCEL_TOKEN=${{ secrets.VERCEL_TOKEN }}
    set-variables: |
      NODE_VERSION=${{ vars.NODE_VERSION }}
      MESSAGE=hello
- uses: jpoehnelt/secrets-sync-action@v2
  with:
    token: ${{ secrets.MY_SECRETS_TOKEN }}
    repositories: |
      octocat/awesome-project
      octocat/my-library
      octocat/hello-world
    delete-secrets: |
      NPM_TOKEN
      VERCEL_TOKEN
    delete-variables: |
      NODE_VERSION
      MESSAGE

Or the action could support both! Both is good, just at the cost of code complexity.

Here's an idea for setting org-level secrets https://docs.github.com/en/codespaces/managing-codespaces-for-your-organization/managing-encrypted-secrets-for-your-repository-and-organization-for-github-codespaces#adding-secrets-for-an-organization

- uses: jpoehnelt/secrets-sync-action@v2
  with:
    token: ${{ secrets.USER_SECRETS_TOKEN }}
    organization: octocat
    vsibility: selected
    repositories: |
      octocat/our-lib
      octocat/awesome-app
    secrets-app: dependabot
    set-secrets: |
      NPM_TOKEN=${{ secrets.NPM_TOKEN }}
Here's how the Deno idea would work (I've used it before)

The idea is that you write your code in TypeScript and then you normally run it with ./cli.ts. But the problem is you need Deno. So guess what? Deno is a single binary that gets downloaded from GitHub Releases. It's really fast and easy to just get the ./deno binary. So what do you do? You spend ~1s downloading it and then you use that binary. This has the bonus side effect of putting you in control of the exact Deno version.

Let me remind you though, that this is an idea. An alternative is to use https://github.com/JasonEtco/build-and-tag-action

Reminder: Bash is available on windows too via git bash. Just use shell: bash.

runs:
  using: composite
  steps:
    - run: '"${GITHUB_ACTION_PATH%/}/cliw"'
      shell: bash
      env:
        INPUT_TOKEN: ${{ inputs.token }}
        INPUT_GITHUB_SERVER_URL: ${{ inputs.github-server-url }}
        INPUT_...: ...

☝ You can also use INPUTS_JSON: ${{ toJSON(inputs) }} if you're feeling concise 😎

#!/bin/sh
# Based on https://github.com/jcbhmr/deno_wrapper
set -e
# https://stackoverflow.com/a/29835459
script_dir=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd -P)
deno_dir="$script_dir/.deno"

# https://manpages.ubuntu.com/manpages/kinetic/en/man1/chronic.1.html
chronic() (
  set +e
  output=$($@ 2>&1)
  exit_code=$?
  set -e
  if [ "$exit_code" -ne 0 ]; then
    echo "$output" >&2
  fi
  return "$exit_code"
)

if [ ! -d "$deno_dir" ]; then
  # https://github.com/denoland/deno_install#readme
  export DENO_INSTALL=$deno_dir
  curl -fsSL https://deno.land/x/install/install.sh | chronic sh -s "v1.33.4"
fi

# https://github.com/denoland/deno_install/blob/master/install.sh#L53
export DENO_INSTALL=$deno_dir
export PATH="$DENO_INSTALL/bin:$PATH"

"$script_dir/cli.ts" "$@"
#!/usr/bin/env -S deno run -Aq
import process from "node:process";
import { readFile, writeFile } from "node:fs/promises";
import assert from "node:assert";
import * as core from "npm:@actions/core@^1.10.0";
import { $ } from "npm:zx@^7.2.2";
Here's what a Python script might look like

This script is nowhere near complete and just scratches the surface. Don't take as end-all-be-all, but instead as a demo that you can do GitHub actions stuff without a massive npm project and without a dist folder in source

Reminder: gh is available on windows, macos, and ubuntu runners! It's a great cross-platform way to interact with github for Bash scripts or other non-lib stuff.

#!/usr/bin/env python3
import subprocess
import os
import tempfile

input_repositories = os.getenv("INPUT_REPOSITORIES")
input_repository = os.getenv("INPUT_REPOSITORY")
input_query = os.getenv("INPUT_QUERY")
input_limit = os.getenv("INPUT_LIMIT")
input_token = os.getenv("INPUT_TOKEN")
input_github_server_url = os.getenv("INPUT_GITHUB_SERVER_URL")
input_dry_run = os.getenv("INPUT_DRY_RUN")
input_app = os.getenv("INPUT_APP")
input_secrets = os.getenv("INPUT_SECRETS")
input_secret = os.getenv("INPUT_SECRET")
input_variables = os.getenv("INPUT_VARIABLES")
input_variable = os.getenv("INPUT_VARIABLE")

assert(input_repositories ^ input_repository ^ input_query)
if query:
  cmd = ["gh", "search", "-L", input_limit, input_query, "--json", "fullName", "-q", ".[].fullName"]
  repositories = subprocess.check_output(cmd).decode("utf-8").splitlines()
else:
  repositories = (input_repositories or input_repository).splitlines()

env_text = (input_secrets or input_secret)
env_file = tempfile.NamedTemporaryFile(mode="w+t")
env_file.write(env_text)

dry_run = input_dry_run == "true"

for r in repositories:
  cmd = ["gh", "secret", "set", "-R", r, "-a", input_app, "-f", env_file.name]
  if dry_run:
    print(" ".join(cmd))
  else:
    subprocess.check_call(cmd)

env_text = (input_variables or input_variable)
env_file.seek(0)
env_file.write(env_text)

for r in repositories:
  cmd = ["gh", "variable", "set", "-R", r, "-f", env_file.name]
  if dry_run:
    print(" ".join(cmd))
  else:
    subprocess.check_call(cmd)

env_file.close()
jcbhmr commented

From browsing some of the other issues, I think using a query: that gives the user control of searching would help alleviate #76 ? Maybe? Maybe not idk.

jcbhmr commented

It might also be worth considering, now that there's secrets, variables, set, and delete: segmenting this into 4 distinct actions: set-secrets-action, delete-secrets-acttion, set-variables-action, and delete-variables-action ? You could even make this action just a "wrapper" around all of those using a pattern like this:

runs:
  using: composite
  steps:
    - if: inputs.mode == 'set'
      uses: octocat/set-variables@v1
      with: { ... }
    - if: inputs.mode == 'set'
      uses: octocat/set-secrets@v1
      with: { ... }
    - if: inputs.mode == 'delete'
      uses: octocat/delete-variables@v1
      with: { ... }
    - if: inputs.mode == 'delete'
      uses: octocat/delete-secrets@v1
      with: { ... }
jpoehnelt commented

👋 Hello @jpoehnelt ! It's me, the person who wanted to contribute some ideas that I had experimented on my own back to this repo (since it's the most popular). Here's some ideas for a @v2 release. These are some pretty drastic changes, and are very focused on improving the usability of this action and making it conform with existing conventions.

Here's a general overview of what I'd like to change. I know this is a lot but hear me out. I'll try to provide a rationale for each change:

  1. Use Deno or https://github.com/JasonEtco/build-and-tag-action. This gets rid of the pesky dist/ folder in source control which is a bit complex to make sure is in sync with code changes. Heck, if it's simple enough, you could even use Bash! (remember Bash is also on Windows runners via git bash) or even Python if you want! The key I think is to scale complexity of managing the project with the complexity of the code in the project. In this case, npm + dist folder is a bunch of extra stuff

🤷‍♂️ I think it isn't that complex actually. Already using ncc and a very simple step in the release workflow: https://github.com/jpoehnelt/secrets-sync-action/blob/master/.github/workflows/release.yml#L13

  1. Use kebab-case inputs since this is what all the official actions use https://github.com/actions. It's also what most other user-made actions use and appears to be "the convention"

👍 Sure, will need to support both for a version or two.

  1. Add aliases for repository repositories and other plurals. This greatly reduces the potential for typos and user frustration.

👎 I disagree on aliases here, I feel it is a bit of an anti pattern. Underlying issue is probably a better validation error/message/suggestion.

  1. Use GitHub Search syntax instead of regex to match multiple repos dynamically. This is more familiar to github users and very easy to test which repos it will apply to! Just use https://github.com/search!

🚫 GitHub search as far as I can tell does not allow filtering by affiliation which is probably always narrower than the regex pattern of owner/name. https://github.com/jpoehnelt/secrets-sync-action/blob/master/.github/workflows/release.yml#L13

Might be worth looking at the graphql api more closely?

  1. Remove (some) linting (config). This is more of a "this is excessive" opinion, but I think that in particular all the eslint and such could be replaced with deno lint or npx xo or something. Even then, there's enough safety with tsc that I don't think this project has reached "big status" to need such fancy linting stuff.

👍 Let's start by just removing everything but recommended + prettier. It is excessive and I probably just copy/pasta from another Google org repo or something.

  1. Use more practical e2e tests instead of unit tests. Sure, ts tests are ok, but they don't compare to uses: ./ to actually run your action in CI. I find that a dry-run: flag plus a "real world" test or two works pretty good.

👍 See https://github.com/jpoehnelt/secrets-sync-action/blob/master/.github/workflows/release.yml#L26, probably can be improved and run on pull requests/forks

  1. Add a comparison blurb to the readme (after everything is said and done) about how this project compares to other secrets sync actions https://github.com/marketplace?type=actions&query=secrets+sync+

👍 This action predated a bunch of things(org secrets, environments, etc) and probably should have some background.

  1. Improve the "feel" of the readme. See https://github.com/Andrew-Chen-Wang/github-wiki-action for what I consider to be "good feel"

👍

  1. Use explicit secrets instead of regex secrets. Sure, it's a tad harder to say "all secrets", but it really really helps for keeping which secrets sync where very explicit. And given that you're usually syncing only 1-5 secrets, this isn't much of a hassle that regex confusion is worth it imo.

🤷‍♂️ Might need to look a little further, see an example. Would be a breaking change?

  1. Use https://github.com/actions/publish-action to atuo-create v2 major tags from a release like v2.0.0

👍 Wish this wasn't necessary. Probably should prioritize given other changes above.

  1. Also support https://docs.github.com/en/actions/learn-github-actions/variables

👍 Wouldn't prioritize given no one has mentioned wanting this.

jcbhmr commented

🚫 GitHub search as far as I can tell does not allow filtering by affiliation which is probably always narrower than the regex pattern of owner/name. https://github.com/jpoehnelt/secrets-sync-action/blob/master/.github/workflows/release.yml#L13

What exactly does this mean? 🤔 I don't fully understand 😵

For example, I've seen this:

image

But I have yet to see an example where the regex couldn't be replaced by either a static list or a dynamic query: "user:octocat topic:my-secrets-topic" or similar

https://github.com/search?q=google%2Fsecrets-sync-action+path%3A.yml&type=code 👈 i can't find any good examples of regex being taken advantage of? most of it's just static lists

jpoehnelt commented

Here is an example: https://github.com/googlemaps/.github/blob/master/.github/workflows/sync.yml#L18

The issue is that we need a list of repositories where the token can actually modify secrets. The search endpoint does not allow this level of filtering. The repos endpoint allows this but doesn't allow filtering by name/owner as far as I can tell.

  • 👍1
  • 😕1
jcbhmr commented

Here's some ideas for other ways to accomplish the end goal (to sidestep this issue -- X/Y problem):

  1. Use organization secrets with limited repo visibility. This is native to GitHub so I'm surprised that it isn't being used by more orgs 🤣 https://docs.github.com/en/actions/security-guides/encrypted-secrets#creating-encrypted-secrets-for-an-organization
  2. Explicitly list the repos. This is more work because you have to edit the sync file, but hey, you'd have to change the Granular PAT to add a new repo anyways or even edit the Settings>Secrets right, so you might as well just edit the sync file
  3. Use a custom tag to dynamically opt-in to a specific secret per repo. This adds an extra tag like googlemaps-secret-sync-api-key or whatever then a query: "org:googlemaps topic:googlemaps-secret-sync-api-key"

As for setting secrets only if the token has access, would it be possible to just print a warning and just continue instead of outright failing if it encounters a permissions error? Then you could just do:

- uses: ...
  with:
    query: org:googlemaps
    secrets: |
      SYNCED_RUN_ID=${{github.run_id}}
      SYNCED_GITHUB_TOKEN_REPO=${{secrets.GOOGLEMAPS_BOT_REPO_TOKEN}}
      SYNCED_GOOGLE_MAPS_API_KEY_SERVICES=${{secrets.GOOGLE_MAPS_API_KEY_SERVICES}}
      SYNCED_GOOGLE_MAPS_API_KEY_WEB=${{secrets.GOOGLE_MAPS_API_KEY_WEB}}

and it could loop through all repos and print a non-fatal warning that you could learn to ignore if you don't want to deal with any of the 👆 other 3 ideas 😅

What I'm getting at here is that regexes seem really wack and I think using a native familiar github feature (search query syntax) is a far better UX experience.

screenshot of the "official way" to do org secrets:
image

jcbhmr commented

👍 Sure, will need to support both for a version or two.

For https://github.com/Andrew-Chen-Wang/github-wiki-action I pushed for a hard v4 that worked out pretty well https://github.com/Andrew-Chen-Wang/github-wiki-action/releases/tag/v4.0.0
that's what major versions are for after all, right? 😅

especially given that i want to switch up the interface, i think it might be ok to have _ vs - breaks like that. you can always just stick with v1 until you want to upgrade which in fact is what a lot of users of that wiki action did since they will never touch that workflow again after getting it to work once.

jcbhmr commented

17. Use explicit secrets instead of regex secrets. Sure, it's a tad harder to say "all secrets", but it really really helps for keeping which secrets sync where very explicit. And given that you're usually syncing only 1-5 secrets, this isn't much of a hassle that regex confusion is worth it imo.

🤷‍♂️ Might need to look a little further, see an example. Would be a breaking change?

As for this, what I would prefer is this:

with:
  secrets: |
    HELLO=${{ secrets.HELLO }}

instead of this:

with:
  SECRETS: |
    ^SYNCED_
env:
  SYNCED_HELLO: ${{ secrets.SYNCED_HELLO }}

my point was that you can't do a ^SYNCED_ as easily, but you're already passing all your secrets in as env vars that you might as well just pass them in as named pairs 🤷‍♂️

and yes, id consider all these things breaking changes/refactors that should be lumped into a v2