-
Notifications
You must be signed in to change notification settings - Fork 92
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
[Discussion] Ideas for v2 #103
Comments
From browsing some of the other issues, I think using a |
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: { ... } |
🤷♂️ 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
👍 Sure, will need to support both for a version or two.
👎 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.
🚫 GitHub search as far as I can tell does not allow filtering by affiliation which is probably always narrower than the regex pattern of Might be worth looking at the graphql api more closely?
👍 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.
👍 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
👍 This action predated a bunch of things(org secrets, environments, etc) and probably should have some background.
👍
🤷♂️ Might need to look a little further, see an example. Would be a breaking change?
👍 Wish this wasn't necessary. Probably should prioritize given other changes above. 👍 Wouldn't prioritize given no one has mentioned wanting this. |
What exactly does this mean? 🤔 I don't fully understand 😵 For example, I've seen this: But I have yet to see an example where the regex couldn't be replaced by either a static list or a dynamic 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 |
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. |
Here's some ideas for other ways to accomplish the end goal (to sidestep this issue -- X/Y problem):
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. |
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 especially given that i want to switch up the interface, i think it might be ok to have |
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 and yes, id consider all these things breaking changes/refactors that should be lumped into a v2 |
👋 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:
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 stuffkebab-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"repository
repositories
and other plurals. This greatly reduces the potential for typos and user frustration.deno lint
ornpx xo
or something. Even then, there's enough safety withtsc
that I don't think this project has reached "big status" to need such fancy linting stuff.uses: ./
to actually run your action in CI. I find that adry-run:
flag plus a "real world" test or two works pretty good.v2
major tags from a release likev2.0.0
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 thesecrets
andvars
inputs for either set/delete operationsAs an alternative, you could make each one a separate input to be more explicit:
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
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
.☝ You can also use
INPUTS_JSON: ${{ toJSON(inputs) }}
if you're feeling concise 😎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.The text was updated successfully, but these errors were encountered: