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

Trigger Dockerfile rebuild/commit on PR #241

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Conversation

trevorcampbell
Copy link
Contributor

@trevorcampbell trevorcampbell commented Sep 13, 2023

This PR would edit the github action to rebuild the docker image when a PR is opened targeting Dockerfile. That would let us easily review PRs that edit the Dockerfile without needing to manually build things locally or interactively before testing.

One TODO: I would need to enable main branch protection to stop accidental commits to Dockerfile directly, which would circumvent the action.

There is one problem with this approach: if someone else were to fork this repo and make a PR that edits the Dockerfile, the workflow would fail. That is very unlikely to happen, so maybe the benefit of being able to check the build on most PRs is worth it.

Another approach would be to have github action build a copy of the book using dockerhub, or if it can't find the image, build the image first locally w/o pushing to dockerhub. That would work with forks, and would enable PR review without needing to build the image locally. Big downside is needing to enable an app (netlify etc) to have permissions in our repos, which I'm not a fan of.

Closes #221

(We should take the same approach in the R book repo)

@joelostblom thoughts?

@trevorcampbell
Copy link
Contributor Author

The more I think about it, the more I'm happy with the way this branch does it. I don't think we've ever (either Py or R) had a fork make a PR to Dockerfile.

@joelostblom
Copy link
Collaborator

I'm in favor of QoL improvements like this that facilitates reviewing! I'm not worried about this not triggering for forks; as you said we don't really use that workflow. I think this gets us closer to what we discussed a long time ago about automatic builds in #44, and the dockerhub approach you described would get us all the way there so I'm intrigued although I don't know how much additional work it would be (I don't mind the enabling of app permissions personally).

@trevorcampbell trevorcampbell marked this pull request as ready for review September 13, 2023 17:22
@trevorcampbell
Copy link
Contributor Author

Alright I'll mark this ready for review, and we can test it out in this repo a bit before doing the same in the R repo.

Copy link
Collaborator

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the diff, the changes here make sense to me. However, I have limited experience with actions workflow, so I wouldn't put much trust in my ability to detect if an undesired change was introduced here.

@trevorcampbell trevorcampbell merged commit 7cac0e8 into main Sep 13, 2023
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.

Dockerfile temporary image build in PRs
2 participants