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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust deploy scripts to archive old releases in a separate branch, move existing releases out of master #2426

Merged
merged 11 commits into from Jan 28, 2022

Conversation

cincodenada
Copy link
Contributor

@cincodenada cincodenada commented Jan 24, 2022

Note: Due to so many files being removed this PR makes GitHub's change viewer unusable, but if you exclude the first commit using the "Commit Range" option, it should be much more reviewable! This link should take you to directly the "Files changed" tab with the first commit excluded:
https://github.com/sinonjs/sinon/pull/2426/files/fe65826171db69ed2986a1060db77944dbc98a6d..HEAD

Purpose (TL;DR)

A proposed solution to #2421. In this solution, master always has just the current release-source directory, and old versions are maintained in a new branch (provisionally named releases), which is maintained alongside master and used to deploy the documentation site.

Solution

In short, this revises the version scripts (including the pre/post) such that instead of copying the release-source into the archive directories in master, it switches to a new releases branch, merges master into that branch, and then does the copying and committing of release-source. It then switches back to the newly-released master.

This PR includes a one-time removal of all the existing release archives from the master branch, and will require a one-time creation of a parallel branch to contain the archives. Creating the new branch is pretty straightforward - it involves branching off of master at the first commit in this PR (the one that removes all the archived releases), and then in the new branch, reverting that commit to restore them. Once that is done, we can move forward merging master into the parallel branch as needed without re-removing everything.

My fork has a full demonstration of an example release of the current repo's head, both with the current method, and with the proposed modifications under this PR. The relevant branches are as follows:

  • status-quo is the product of running npm version v12.0.2-status-quo on the current master commit, plus committing the uncommitted CHANGELOG.md changes that are left over (see "Other Fixes" below). This is (I think) what master would look like after a release currently.
  • separate-releases-demo is the product of running npm version v12.0.2-releases-demo on the separate-releases branch*. This is what master would look like after a release under this proposal.
  • releases is the new parallel branch used in this example, and demonstrates how that branch would look after the v12.0.2-releases-demo release and archiving.
  • The releases-base tag is how the releases branch should be initialized before running npm version with the new setup, and thus serves as the base for releases.

Perhaps more useful than looking at the individual commits would be the network tab or your favorite branch viewer:
sinontig

Also, I'm super not attached to the name for the releases branch, it could be archive or doc-site or whatever.

The most recent commit in this PR is temporary and should not be merged - notably, it adds the --dry-run flag to avoid actually publishing a version. This was initially because I can't do that and it broke my builds, but you probably also don't want to run it for real under testing either! Assuming this gets merged, I'll remove that commit before we do.

Other Fixes

Along the way, I also fixed a few other things that I noticed, particularly around the changelog - I see y'all recently updated how that is generated, and there were a couple rough edges left over, I think. Namely:

  • We were manually running prettier on CHANGELOG.md after user-edits as part of npm version, but then not actually committing the prettier fixes - and they indeed seem to have never made it into the repo.
  • We were ignoring the changelogs in .prettierignore, which seemed counter to linting it in the version pipeline. Removing them from the ignore didn't introduce massive changes or anything, so I did so.
  • Because we updated some auxiliary version bumps as part of postversion, the tagged release commit didn't have all the version updated everywhere, and the version script was adding a follow-up commit that did that tidying. This PR moves that logic into a new version.sh script, so that all the versioning updates are present in the tagged commit.
  • Because of how we were constructing the changelog page on the docs site, there were two headers, "Changelog" and "Changes" - I removed the second header here, since the duplication seemed unintentional and unnecessary.

How to verify

Note: These directions will not actually publish a package to NPM (Per above, I added the --dry-run flag in this PR) but it will actually create branches and tags in whatever repo you're pushing to!

  1. Check out this branch
  2. Create a releases branch, either by branching from this branch and reverting as detailed above, or simply setting it to the releases-base tag from my fork
  3. Switch back to this branch
  4. npm install
  5. npm version v12.0.2-whatever-you-want (again, this is a dry-run, I wouldn't blame you for double-checking the script first tho 馃槃)
  6. In the changelog editor that comes up, add something that needs to be linted to the changelog. Easy test: surrounding a word with asterisks instead of underscores to make it italic.
  7. Verify that the release and tag is created and the releases are added to the releases branch, but not master
  8. Verify that the same changes are pushed up to the remote repo

Other stuff

If you want to reset the state of your repo after testing, I used this reset.sh script to clean up the local and remote state during my own testing:

TAG=${1:-v12.0.2-releases-demo}

if [ -z "$(git diff --stat $TAG)" ]; then
  git reset $TAG^ && git stash
  git push -f origin
fi
git tag -d $TAG
git push origin :$TAG
git branch -f releases releases-base
git push -f origin releases
if [ -d "docs/_releases" ]; then
  echo "Uh-oh! Leftover cruft! Removing"
  rm -r docs/_releases
fi

In the separate-releases-demo branch you can't see my original additions to CHANGELOG.md before linting, because it's now linted before committing. For reference, the most recent commit in the status-quo branch shows what I added to CHANGELOG.md, and what changes the linter made.

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

These archives made it difficult to find things in the GitHub interface,
and take up a lot of space in a checked-out repo for something that is
not useful to most people checking out the repository.

The main purpose of these archives is to make old versions and
documentation available on the Sinon website that is run out of this
repo. This can be supported by using a separate branch for website
releases, and to maintain the archives.

Following this commit, the `npm version` scripts will be updated to
automatically handle archiving the releases in the new releases branch
and keeping it up to date with master.

Also remove the directories we removed from .prettierignore, since they
don't exist any more.
This updates the `postversion.sh` script to merge master into the new
`releases` branch and then copy and commit the version archives there,
instead of into master, so that the archives aren't cluttering up the
master branch unnecessarily.
Previously we were updating the changelog and doing other auxiliary
version bumps in the post-version script, which meant they weren't
included in the tagged release commit, and required a followup commit.

This moves the version-bumping changes into a new `version.sh` script,
so that those changes show up in the tagged version commit.
We were manually running prettier on CHANGELOG.md during release but
ignoring it in .prettierrc - this may have been a holdover from before
Prettier had the proseWrap option, which would have wreacked havoc with
the changelog, but the current version handles things just fine.
We shouldn't ever have conflicts here, so we can just auto-merge
$(<CHANGES.md) wasn't working on my machine, I think because it's a
bash-ism, and my machine was running it under sh. I also took the chance
to do something more useful than just `cat` by removing the double
header that was previously on this page.
Also make version.sh a bash script like the others - this would have
fixed the update-change-log-page script, in retrospect, but I think it's
worth getting rid of the double header anyway.
Otherwise when we switch back to the source branch we'll have these
files left dangling, which was what we were trying to avoid in the first
place
Stringing commands together like this makes it so if the push doesn't
work, we keep going, which we don't want!
This makes the script work on my machine, since I of course can't
actually publish to sinon's npm package

Also skip postbuild because chrome tests aren't working on my machine
and `npm install` doesn't fix it
@cincodenada cincodenada marked this pull request as draft January 24, 2022 03:17
@cincodenada
Copy link
Contributor Author

cincodenada commented Jan 24, 2022

This is ready for review, but GitHub doesn't have a "ready for review but not ready to be merged" state.

I marked this as a draft to be extra safe, because it shouldn't be merged as-is with the npm publish disabled and I'm not sure how it will interact with actual deploy of the website.

@cincodenada cincodenada marked this pull request as ready for review January 24, 2022 03:18
@cincodenada cincodenada marked this pull request as draft January 24, 2022 03:18
@cincodenada cincodenada changed the title Separate releases Adjust deploy scripts to archive old releases in a separate branch, not in master Jan 24, 2022
@cincodenada cincodenada changed the title Adjust deploy scripts to archive old releases in a separate branch, not in master Adjust deploy scripts to archive old releases in a separate branch, move existing releases out of master Jan 24, 2022
@fatso83
Copy link
Contributor

fatso83 commented Jan 25, 2022

@mroderick Worth a peek!

@cincodenada
Copy link
Contributor Author

cincodenada commented Jan 25, 2022

I just realized this PR is impossible to review on GitHub by default - but you can use the Commit Range feature to ignore the first commit that removes 2500+ files, and review the rest of the changes. This link should take you to the changes tab with the relevant commit range pre-selected, I've added this to the description as well!
https://github.com/sinonjs/sinon/pull/2426/files/fe65826171db69ed2986a1060db77944dbc98a6d..HEAD

@fatso83
Copy link
Contributor

fatso83 commented Jan 27, 2022

I am good with this! Works fine once I read your instructions 馃槃

@fatso83 fatso83 marked this pull request as ready for review January 27, 2022 10:25
@fatso83 fatso83 merged commit 4171046 into sinonjs:master Jan 28, 2022
fatso83 added a commit that referenced this pull request Jan 28, 2022
@fatso83
Copy link
Contributor

fatso83 commented Jan 28, 2022

Oy, seems it was not such a great idea to forget firing up the docs site on localhost before merging this. Not sure how I managed to forget that one, but this broke the docs site big time. This is my fault for not checking of course, but if I do not manage to figure this out quickly, it would be great with a hand.

All the releases, that was moved out of /docs/, are not part of the deployed page.

Ref #2432

@fatso83
Copy link
Contributor

fatso83 commented Jan 28, 2022

fixed, there were just a few minor issues. one was merge conflict markers left after failing to realize I needed to not squash this, but keep history to get the whole releases branch thingie to work correctly and then needing to change settings for how Github Pages are built.

@cincodenada
Copy link
Contributor Author

cincodenada commented Jan 28, 2022

Ah yeah, in retrospect I should have asked if there was some time where I could be on hand for the merge in case things went wonky, cause there were a lot of moving parts here - as-is it was something like 4am my time at merge-time and I was definitely not awake. Sorry for all the mess!

I also should have thought about the squashing, I forgot that that y'all usually squash-merge - because yeah, the creation of the releases is some fairly delicate git-wrangling, and as you discovered the hard way you need at least the removal commit to exist on its own as a starting point for the releases branch.

I'm reviewing the history now to make sure things should be in a good state moving forward, now that all the resulting conflicts and things are resolved, but immediately, it looks like the npm release didn't actually happen, because I didn't communicate clearly enough that the temporary commit should be removed before merging (along with the commit from the test release)

The end result is that the --dry-run flag from testing is still on npm publish. I've opened #2434 to fix that for future releases, but I think you'll need to manually run npm publish to retroactively release v13.0.0

@fatso83
Copy link
Contributor

fatso83 commented Jan 28, 2022

Hah, good catch!

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.

None yet

2 participants