jpoehnelt / secrets-sync-action

A Github Action that can sync secrets from one repository to many others.
https://github.com/marketplace/actions/secrets-sync-action
Apache License 2.0
314 stars 93 forks source link

[Discussion] Ideas for v2 #103

Open jcbhmr opened 1 year ago

jcbhmr commented 1 year ago

šŸ‘‹ 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 ```yml - 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: ```yml - 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 ```yml - 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`. ```yml 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 šŸ˜Ž ```sh #!/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" "$@" ``` ```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. ```py #!/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 1 year ago

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 1 year ago

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 1 year ago

šŸ‘‹ 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 1 year ago

šŸš« 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 1 year ago

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.

jcbhmr commented 1 year ago

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 1 year ago

šŸ‘ 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 1 year ago

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