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

Autogenerate chapters on commit to master #653

Merged
merged 3 commits into from
Feb 14, 2020
Merged

Conversation

tunetheweb
Copy link
Member

This adds a GitHub action so any pushes to master, cause npm run generate to automatically run and check in any changes.

This makes it a lot easier to accept small typo fixes as currently have to either ask the PR-raiser to run this and include those changes, or do it yourself, or leave it to the next poor sucker that runs it to pick up this change.

Doing this as part of the pull request is not easy, and even then can lead to merge conflicts, so I think it is better doing it on master anyway post-commit.

@tunetheweb tunetheweb added the development Building the Almanac tech stack label Jan 30, 2020
@rviscomi
Copy link
Member

This looks super powerful and I agree it's a great tool to assist first-time contributors.

What are the risks of a process like this? I'm curious if this could get master into a bad state if there's a bad generation.

@tunetheweb
Copy link
Member Author

tunetheweb commented Jan 30, 2020

Yeah I really wanted to do this on Pull Request so we could review changes but can't see how to do this. Weird that the calibre plugin is able to do this and I tried to disect the code to see how that did that but gave up as looks like it uses the git api and creates a new tree and then merges it in and all that looked way too complicated.

We can change it to happen on pushes except those for master so it could be done BEFORE raising a pull request so it's part of that. The problem with that is each time you push to GitHub it will change things on that branch, and, you'll need to pull again to get the changes or your next push won't work as it says it's out of sync. That could get tiresome depending on whether you push to origin a lot or not.

I did raise a query on this issue: actions/checkout#124 (comment) and maybe should wait to see if we get an answer on that to see if there's a better way? But wanted to get the code up there for discussion (I never know whether to raise and Issue for discussion or a PR but like seeing the code so like PRs - but probably should have raised as a draft to have the discussion?).

In theory it should be safe, as we are only running the npm run generate script (that I presume we have confidence in!), and I only add the templates to the commit:

git add src/templates

Could cut that down further to only add the chapter template files and the sitemap.xml if we wanted to be more cautious as those should be totally regeneratable if anything goes wrong:

git add src/templates/*/*/chapters/
git add src/templates/sitemap.xml

That's probably a good change to reduce risk, so will add that to this PR.

They are also added as a commit so worst comes to the worst, we can always look at the change after the fact, as we can any other commit, and then reverse it if we want.

The one issue I've seen is if you make a change to an .md file, then run npm run generate, then make some more changes to an .md file but this time don't regenerate and then submit. Then it sometimes complains of missing changes (merge conflict?). It works fine if you either never do an npm run generate as part of a PR or always do an npm run generate as part of a PR and only seems to be an issue when giving a stalenpm run generate . But all that happens in that case, is it bails out before committing any changes so it'll be picked up on next run. So not bad and just results in a failed Action run.

It is definitely an annoying problem though that this PR looks to solve. I merged 3 PRs yesterday for simple typos and know we need to do a chapter generation for them. I really don't want to discourage people submitting those but it is tiresome asking for npm run generate (and I only do this for people who are, or look like they will be, regular contributors) or waiting for someone to ask later why they have extra changes in their PR that they didn't do.

Only commit the chapters and sitemap which are re-generatable in case of issues.
@tunetheweb tunetheweb merged commit 776db0c into master Feb 14, 2020
@tunetheweb tunetheweb deleted the autogenerate_chapters branch February 14, 2020 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Building the Almanac tech stack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants