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

Take generated files out of version control #228

Closed
jgonggrijp opened this issue Aug 24, 2020 · 10 comments
Closed

Take generated files out of version control #228

jgonggrijp opened this issue Aug 24, 2020 · 10 comments
Labels
before modules This needs to be tackled before modularization (temporary label, see #220) enhancement

Comments

@jgonggrijp
Copy link
Contributor

I agree with #211 (comment) by @joshuacc. Same applies to the dist files. We can tell git to delete them, add them to the .gitignore, and then continue business as usual.

@jgonggrijp
Copy link
Contributor Author

@joshuacc I was about to do this, mostly because I'm getting annoyed at having to uncheck the modified dist files every time I commit through my editor. However, there is a catch.

If we add the generated files to the .gitignore and remove them from VCS, we have to be very careful to undo these changes (i.e., remove the .gitignore lines and re-commit the generated files) the first time we merge master into gh-pages. At least, I hope we're going to adopt such a procedure for subsequent releases (see #172).

There is also the disadvantage that if people download a zip of the source code, they will get no documentation until they install the dependencies and run the build steps. There is something to say for having everything committed like in Underscore, too. In Underscore, there is even a pre-commit hook that keeps the non-minified dist files in sync with the source, so you can always download a bleeding-edge version from the website.

So, where do we take this?

@joshuacc
Copy link
Contributor

I see a couple of different concerns here.

First, the generated JS files. Those can be generated as part of a pre-publish script instead of a pre-commit. Then there is no need to keep them in git at all.

Second, the generated HTML docs. I don't think those need to be present in git or the published package. The markdown docs are already available there. But if you do want them published in the npm package, just do it as part of the pre-publish script.

@jgonggrijp
Copy link
Contributor Author

While I don't necessarily disagree, I think it is a bit more nuanced than you paint it:

  • There is no need to keep the generated JS files in git, unless you want to offer a "bleeding edge" download of the current tip of the master branch. This isn't currently the case, so not much is lost if we remove the generated JS files from git entirely, but Underscore does have such a download option. I don't really mean to argue for this, just pointing out that a choice not to commit the JS bundles is also a choice not to publish a bleeding edge version.
  • More importantly, I agree that the Markdown files are sufficient for people who download the package, but the gh-pages branch really needs the HTML. If we never commit the HTML files except on the gh-pages branch, that may potentially be a recipe for mistakes.

I'm going to take a dive into GH Actions in order to enable CI. While I'm at it, I'll investigate whether it might could help us with this issue as well. It's easier to make choices when all the options are on the table.

@joshuacc
Copy link
Contributor

I'm honestly fine with you doing this however you want, so take the following as you will. :-)

For the generated JS files, I don't see any benefit to keeping them in git. Bower is deprecated. If we want to offer a non-npm method of downloading the minifified version, that could just be hosted on GitHub pages as part of the docs site.

For GitHub pages generation, this action should help: https://github.com/marketplace/actions/deploy-to-github-pages

@jgonggrijp
Copy link
Contributor Author

Thanks for the trust, and for the helpful pointer. :-)

I've exercised with CI (#230) and I plan to add CD in one go. Despite your kind gesture, I'm still going to ask your opinion, because I prefer to set up a scheme that both of us really like if I can help it.

The plan I'm going to describe may appear a bit complicated, but hopefully you'll agree it all makes sense. My main objectives are the following:

  • Reduce work and opportunities for mistakes for the maintainer.
  • Commit docco-generated HTML to gh-pages but not to master.
  • Keep the risk of inconsistent deployment at a minimum (published to NPM but not to gh-pages or vice versa, version tag on the wrong commit, etcetera).

As I currently envision it, cutting a new release would consist of the following steps.

  1. Maintainer creates a release/x.y.z branch based on master.
  2. Maintainer bumps the version in the package.json to x.y.z, without creating a release tag. So yarn version --no-git-tag-version with the --major, --minor or --patch flag.
  3. Maintainer updates the change log and the documentation (markdown source) as necessary.
  4. Maintainer checks that tests, docs and annotated source (which I intend to add in the future) all look good.
  5. Maintainer submits PR for the release/x.y.z branch. At the very least, in order to trigger CI and to leave a trace, possibly also so that another maintainer can verify the change log.
  6. Maintainer merges the release/x.y.z into two branches: master and prepublish. prepublish will be a persistent branch, intermediate between master and gh-pages.
  7. CD workflow checks out the prepublish branch and attempts to execute the following steps in order, bailing out on the first error.
    a. yarn grunt dist docco tocdoc.
    b. Commit the build output.
    c. yarn publish (or npm publish if this works better with GH Actions).
    d. Tag the tip of prepublish as version x.y.z.
    e. Merge prepublish into gh-pages.
  8. If step 7 failed, maintainer adds fixes to release/x.y.z and tries again from step 6.
  9. Maintainer deletes the release/x.y.z. branch. The next merge into prepublish will have a version number greater than x.y.z.

To make this work, some one-time preparations would be required:

  • On master, run git rm -r dist docs/*.html index.html, add these patterns to the .gitignore and commit.
  • Start the prepublish branch based on master. Immediately revert the previous commit on this branch.
  • Rename the existing gh-pages and create a new gh-pages based on prepublish (Base gh-pages off of master? #172).
  • Add a .nojekyll to the new gh-pages branch because of gh-pages is hiding the internal modules in the modular annotated source jashkenas/underscore#2873.
  • Steps 1-6 largely align with the release convention of git flow, except that master is called prepublish and develop is called master. Maintainers can streamline steps 1 and 6 by installing the git-flow tool, running git flow init once with adjusted branch names, and then using git flow release start and git flow release finish -kn for steps 1 and 6, respectively.
  • Enabling the CD workflow to publish to NPM on our behalf will require adding one or more keys to the secrets in the repository settings, so they can be referenced as a variable name in the workflow file. I cannot access the repository settings; if you can't either, we may need to seek Jeremy's help.
  • Since this infrastructure enables anyone with write access to publish to NPM, we may want to clean up the list of people with write access. Again, this might require help from Jeremy.

Please let me know what you think. If you have any hesitation, even a very slight one, I want to know it. I will not proceed until I have your feedback.

@joshuacc
Copy link
Contributor

joshuacc commented Sep 5, 2020

It looks a little more complicated than I prefer, but I have no fundamental objections. My only request is that the process gets documented in a markdown checklist somewhere so that maintainers can follow through on each item without losing where they were.

@jgonggrijp
Copy link
Contributor Author

This exact process is not sacred to me, as long as we meet the goals. So if you have any ideas, even if they don't entirely meet the goals, I'd be interested to hear them. I'd prefer simpler, too, but haven't been able to come up with a simpler flow yet.

@joshuacc
Copy link
Contributor

joshuacc commented Sep 6, 2020

I suggest going with this. After it has actually been used a time or two, I think any important improvements will become obvious.

@jgonggrijp jgonggrijp added the before modules This needs to be tackled before modularization (temporary label, see #220) label Oct 15, 2020
jgonggrijp added a commit that referenced this issue Oct 15, 2020
This reverts commit e166a00.
On the prepublish branch, we do want these files to be committed.
@jgonggrijp
Copy link
Contributor Author

@joshuacc FYI, I have executed the branch changes as described above. We now have:

  • master, which is just what it was before but without generated files.
  • prepublish, which in the future will be the branch that triggers CD.
  • gh-pages-old, which is the old, diverged gh-pages branch.
  • gh-pages, a new branch that descends from master and prepublish so we can merge changes into it.

If you fetch and track these branches locally, you can run git flow init with prepublish as the production branch, master as the "next release" branch and v as the version tag prefix. No hurry though, this can wait.

I'll continue with CD in #230 soon.

jgonggrijp added a commit to jgonggrijp/underscore-contrib that referenced this issue Oct 30, 2020
jgonggrijp added a commit to jgonggrijp/underscore-contrib that referenced this issue Oct 30, 2020
jgonggrijp added a commit to jgonggrijp/underscore-contrib that referenced this issue Dec 13, 2020
jgonggrijp added a commit to jgonggrijp/underscore-contrib that referenced this issue Dec 13, 2020
jgonggrijp added a commit to jgonggrijp/underscore-contrib that referenced this issue Dec 13, 2020
jgonggrijp added a commit to jgonggrijp/underscore-contrib that referenced this issue Dec 17, 2020
jgonggrijp added a commit to jgonggrijp/underscore-contrib that referenced this issue Dec 17, 2020
jgonggrijp added a commit to jgonggrijp/underscore-contrib that referenced this issue Dec 17, 2020
jgonggrijp added a commit that referenced this issue Dec 17, 2020
@jgonggrijp
Copy link
Contributor Author

I'm still tinkering with continuous deployment (see #238), but the files mentioned in the ticket title have been taken out of VCS in #230. Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
before modules This needs to be tackled before modularization (temporary label, see #220) enhancement
Projects
None yet
Development

No branches or pull requests

2 participants