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

Add squash merges option for editorial workflow #1330

Merged
merged 15 commits into from
May 14, 2018
Merged

Add squash merges option for editorial workflow #1330

merged 15 commits into from
May 14, 2018

Conversation

delucis
Copy link
Contributor

@delucis delucis commented Apr 29, 2018

Summary

Closes #1328

Exposes a backend.squash_merges option, which allows users to configure the merge method used when merging Github pull requests. The standard behaviour remains unchanged, but setting squash_merges: true will use the squash merge method instead (see Github API docs).

Why?

When using the Editorial Workflow, every small change generates at least two commits in the repository history with standard merges, more if content was edited several times before publishing. For content-heavy sites, this can mean the commit history is overloaded with (pretty uninformative) merge commits. Squash merges allow users to minimise clutter in the commit history if they want to.

Considerations

I’m not sure how this fits together with planning for other backends.

Test plan

  • npm run test passed
  • npm run lint threw 281935 problems (245884 errors, 36051 warnings), which I’m hoping my changes didn’t cause 😂
  • Tested in a branch of a Nuxt + Netlify CMS site deployed to Netlify (using git-gateway) and seems to be working, would be happy to follow up suggestions for further testing

Description for the changelog

Add squash merges option for editorial workflow

A picture of a cute animal (not mandatory but encouraged)

An okapi 😀

An Okapi standing in long grass

@verythorough
Copy link
Contributor

verythorough commented Apr 29, 2018

Deploy preview for netlify-cms-www ready!

Built with commit b644032

https://deploy-preview-1330--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

Deploy preview for cms-demo ready!

Built with commit c6ab176

https://deploy-preview-1330--cms-demo.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Apr 29, 2018

Deploy preview for cms-demo ready!

Built with commit b644032

https://deploy-preview-1330--cms-demo.netlify.com

@Benaiah
Copy link
Contributor

Benaiah commented May 2, 2018

@delucis re: fitting in with other backends: part of the backend API discussion has been around separating options that are specific to a given backend from options that would apply to any backend used by the CMS1. This is currently unimplemented, so I think the current location is fine, but I think we may want to document this as a preview feature so that we're not committed to maintaining the current key location when it may change soon.

So basically, there are some considerations around other backends, but I don't think there's any related changes needed in this PR before it's merged.

1 e.g., something like the following:

backend:
  name: github
  settings:
    squash_merges: true # this setting is github-specific

It probably won't look exactly like this, but that's the general idea.

@delucis
Copy link
Contributor Author

delucis commented May 2, 2018

@Benaiah Thanks! If this should be documented as a “preview feature”, should I document it on the “Beta Features!” page? Or just add a note where it is at the moment?


Out of curiosity, just looked into how squash merging would work with the BitBucket and GitLab APIs, so posting that here for future reference:

  • BitBucket pull requests can be squash merged by passing merge_strategy: squash when merging [Docs →]
  • GitLab merge requests have a boolean squash property that is set directly on the request (at create/update) rather than when the request is merged [Docs →]

@Benaiah
Copy link
Contributor

Benaiah commented May 2, 2018

@delucis I think the Beta Features page would be the best spot (@erquhart @verythorough what do you think?). Thanks for looking into GitLab and BitBucket! It looks like it should be straightforward to support this feature on those backends as well in the future.

@delucis
Copy link
Contributor Author

delucis commented May 3, 2018

Copy link
Contributor

@verythorough verythorough left a comment

Choose a reason for hiding this comment

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

Such a great feature addition! I added one comment on the doc.

@@ -77,3 +77,10 @@ import styles from '!css-loader!sass-loader!../main.scss'
CMS.registerPreviewStyle(styles.toString(), { raw: true })
```

## Squash merge GitHub pull requests
When using the [Editorial Workflow publish mode](/docs/configuration-options/#publish-mode) with the `github` or `git-gateway` backends, Netlify CMS creates pull requests to hold drafts while they are being edited and then merges them into your main branch when you press publish. By default, these are merged preserving the history of all the individual commits (i.e. changes where you pressed save). If instead you would like [“squash”](https://help.github.com/articles/about-pull-request-merges/#squash-and-merge-your-pull-request-commits) these commits, reducing clutter in your repository’s history, you can set the following option in your `config.yml`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Great description! The one suggestion I would make is to simplify the second-to-last sentence. Parentheticals and Latin abbreviations like i.e. can be difficult for translators, etc. I suggest ending the sentence after "individual commits" and changing the parenthetical to a sentence, like:

In other words, you'll have a commit in your history for every time you pressed the save button.

I would also suggest adding a paragraph break before the last sentence ("If instead...").

@delucis
Copy link
Contributor Author

delucis commented May 5, 2018

@delucis
Copy link
Contributor Author

delucis commented May 11, 2018

@Benaiah @verythorough Let me know if there’s anything I can do to improve this before it’s merged!

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

Looks great! Just one change request and this is good to merge.

@@ -50,6 +50,9 @@ export function validateConfig(config) {
if (typeof config.getIn(['backend', 'name']) !== 'string') {
throw new Error("Error in configuration file: Your `backend.name` must be a string. Check your config.yml file.");
}
if (!isBoolean(config.getIn(['backend', 'squash_merges'], false))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave this out, trying to avoid validation of beta features, plus this is specific to one backend. After 2.0 I expect we'll provide a way for specific backends to validate their configuration, as a similar effort is underway for widgets.

This reverts commit 93d20cc.
@delucis
Copy link
Contributor Author

delucis commented May 12, 2018

@erquhart Thanks! Removed that block.

@erquhart erquhart dismissed verythorough’s stale review May 14, 2018 14:48

Rewrote section, comments addressed.

@erquhart
Copy link
Contributor

Thanks again @delucis!

@erquhart erquhart merged commit 0408702 into decaporg:master May 14, 2018
@delucis delucis deleted the api-merge-method-config branch May 14, 2018 16:08
brianlmacdonald pushed a commit to brianlmacdonald/netlify-cms that referenced this pull request May 23, 2018
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.

4 participants