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
Enabling PR Previews with Azure Storage #189
Conversation
❌ Deploy Preview for romantic-austin-f85522 failed.
|
Woo! Progress. Just need to check why some files are missing. |
Latest preview works. Adding parallelism to the upload so it can go a little faster. Then I'm leaving it for tonight and picking up tomorrow. |
This is a neat idea. Your end-to-end flow makes sense to me. The main area I was pondering was security. With anyone in the world being able to create a PR from a fork and trigger this, I was trying to think through if there's any security vulnerabilities potentially introduced with a DIY preview solution. Anything that perhaps netlify was handling. I haven't come up with anything specific so maybe there isn't any concerns? |
GitHub Actions abuse is a definite concern. I believe we are relatively safe here though because the secrets are not accessible from forked repositories and any runs in those repositories run in those contexts. The ODIC credentials for Azure will only operate if the workflow is run from the base repository. While this is a public repository, forks with PRs from those also should not automatically run using the I believe we do not have any concerns along those lines but it is a good thing to verify. |
This actually isn't a concern. The only time the secret values will be available is if the PR is generated from a branch within the repo. If the PR is from a branch in a forked version of the repo, the Azure secret values will not be populated. Since only org members have write access, the only time a PR comes from inside the repo, should be from helper bots or actual Ocelots. @nerddtvg instead of failing explosively, can you add an if conditional to the PR workflow so that it will only run if the required env variables exist? The rest looks good to me! |
@PMByrne What env vars are you referring to? |
The 3 azure ones. They should not exist if the PR comes from a forked repo. |
@PMByrne A test was done with an empty string in place of an empty secret and the job was skipped: https://github.com/ocelotconsulting/ocelotconsulting.github.io/actions/runs/6026431339/job/16349350518 At this time, I think the work on this is completed. I've already disabled Netlify so it would stop bugging us on this PR. Technically we need to test deleting the storage account but that will be done with the PR closure, and I don't expect an issue there. |
The preview storage account has been deleted. |
This PR implements temporary Azure Storage based static sites for PR previews. A new storage account will be created for each PR (
ocelotprpreview##
). Every run of the workflow will build and upload the latest version so updated PRs will push changes similar to today's setup.When a deployment is pushed, a comment on the PR will be added with a link to the static website.
Once a PR is closed, whether merged or not, the storage account will be deleted. When the account deleted, a comment will be added stating that. If the storage account cannot be deleted, a comment will note that as well.
The specific app registration for this only has
Reader
andStorage Account Contributor
access to a single RG. This will ensure it cannot be manipulated to break something else in the sandbox environment.The storage accounts will be tagged to indicate they are managed by GitHub Actions with a reference to the appropriate PR number.
Fixes #183