Skip to content
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

Added requierments that secret is present before running job #90

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Starmania
Copy link
Contributor

Builds with GitHub Action take times. And this is a cost. So now, if a secret variable or a environement variable is missing, it will not run the associated job.

@pimterry
Copy link
Member

pimterry commented Sep 4, 2023

Thanks for this PR too, but I don't want to go down this specific route I think, for a few reasons:

  • It makes the action definitions all significantly more complicated
  • It increases the risk that a typo or setting change will silently disable one of these steps (I would rather they did fail for real!)
  • Assigning these values like this makes it more likely these secrets could be accidentally leaked in build output
  • In general, ensuring the CI job is perfectly usable for forks isn't something I want to to officially support - it's nice to do that of course, but it's a chunk of work, it can be difficult in some cases, and it makes it challenging to make changes & evolve the tooling for this repo in future. I'm open to improvements, but I think for many fork cases the fork will need to take responsibility for configuring their own CI job for their specific needs from scratch (or just disabling the CI job entirely).

Can you explain more about how you're using your fork, and how you'd like the CI job to work? Maybe there's an alternative solution that would stop things breaking, but still keep this code simple and avoid those risks.

@Starmania
Copy link
Contributor Author

Starmania commented Sep 6, 2023

I understand your concerns, but why not so use a specific action to build and push to prod only when needed ?

Assigning these values like this makes it more likely these secrets could be accidentally leaked in build output

You don't need to worry for this, because GH filter log with secrets already defined as should never appear, at any moments. It's also why I made a separate job do handle this.

@pimterry
Copy link
Member

pimterry commented Sep 7, 2023

I understand your concerns, but why not so use a specific action to build and push to prod only when needed ?

No, I definitely want to to stick with continuous deployment - automatically deploying everything at the moment it's committed is extremely effective for quick iteration, capturing rapid feedback, and finding bugs etc. It's an enormous boost to the project, and it saves a lot of working & planning too.

You don't need to worry for this, because GH filter log with secrets already defined as should never appear, at any moments. It's also why I made a separate job do handle this.

Yes, this is a nice backup, but it's a last-chance protection, it's not a security feature that's designed to be relied on (GitHub's own docs have repeated caveats warning about this and the cases where it won't work). There are many ways to leak secrets, and we should be careful with them where possible (and it is possible to do so here).


Can you explain more about how you're using your fork, and how you'd like the CI job to work for you personally? Maybe there's an alternative solution that would stop things breaking, but still keep this code simple and avoid those risks.

@pimterry pimterry force-pushed the main branch 2 times, most recently from e0e350c to 1d713c8 Compare May 7, 2024 20:22
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.

2 participants