Fix multi branch github deployes by appending branch name for cms pul… #1494

richtera · 2018-07-10T18:39:07Z

Added support to name space the CMS_BRANCH_PREFIX by including the branch name for non-master branches. This allows multiple deployments to each user their own branch and their own pull requests.

verythorough · 2018-07-10T18:40:39Z

Deploy preview for netlify-cms-www ready!

Built with commit 3691a3a

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

verythorough · 2018-07-10T18:40:47Z

Deploy preview for cms-demo ready!

Built with commit 3691a3a

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

erquhart · 2018-07-31T22:13:57Z

src/backends/github/implementation.js

@@ -138,7 +138,7 @@ export default class GitHub {
      const promises = [];
      branches.map((branch) => {
        promises.push(new Promise((resolve, reject) => {
-          const slug = branch.ref.split("refs/heads/cms/").pop();
+          const slug = branch.ref.split(`refs/heads/${this.api.cms_branch_prefix}`).pop();


This will break sites that don't use master and have existing editorial workflow entries. This kind of backwards compatibility bug is the primary concern with a change like this. We almost need a way to find and transition existing entries, or by some other means ensure that that the change is transparent for existing entries.

@erquhart Doesn't line 14 in API.js address this concern?

this.cms_branch_prefix = this.branch === 'master' ? 'cms/' : `cms-${this.branch}/`;

richtera · 2018-08-01T06:17:18Z

It should not affect people that don't use master. It will just keep changes for each main branch separately. Currently things will get mixed and if you have the CMS attached to two different sites each bound to a different branch then changes for one branch get committed to the last branch that commits the workflow. So it will move changes across branches and remove them from the branch they belong to in some cases.
We have been using this patch in production and it works well

richtera · 2018-08-01T07:49:18Z

Rebased.

richtera · 2018-08-01T07:52:18Z

I had to rebase which makes the original comment invalid in this thread. Basically the one side of the changes is for the write site into github causing it to make commits of cms- for master and cms-BRANCH- for other branchs. The second file is for reading so it reads the last cms- commit matching the branch it came from.

erquhart · 2018-08-01T14:56:31Z

You're changing the unpublishedEntries method, which fetches unpublished entries.

Example of how this breaks things:

a project has existing unpublished entries
the project is using the develop branch
existing unpublished entries are on branches prefixed with cms/
with this change, the CMS will look for branches prefixed with cms-develop/
no existing entries will show, existing unpublished entries will be inaccessible

richtera · 2018-08-02T09:18:29Z

@erquhart Yes, but that's the whole point of this patch. When you have two deployments one against master and one against dev with each the cms configured then making changing in dev or master will show up as unpublished entries on the other branch and will get committed on the wrong branch. We have the client work on a deployment in master and we ourselves work on a deployment in dev. When the client has items in their workflow they will see ours and visa versa. So if in dev I work on a certain post of make changes I will all of a sudden see modifications the client has made to the same post and when I merge my changes I will take theirs with it removing them from their change set and workflow.

tech4him1 · 2018-08-02T13:39:52Z

@richtera If I understand correctly, the problem @erquhart mentions would come when someone is using a non-default branch (production, for example), and that is their main deployment -- they are not using seperate CMS instances. We need to handle both the case you are addressing and this case.

richtera · 2018-08-02T14:01:13Z

Maybe we could then make a little config section in the back end that resolved specific CMS prefix names son only people that need it will get the separate bwhavior

richtera · 2018-08-03T07:49:08Z

I changed the implementation to use a custom_branches object to specify the cms prefix. So that way it will not change existing implementations but will allow anyone to configure a multi branch multi deployment setup.
Example:

backend:
  custom_branches:
    master: cms-master
    dev: cms-dev

erquhart · 2018-08-04T12:48:45Z

That makes sense, but it's probably worth it to keep this implementation detail internalized. What we need is a better approach to handling metadata, one that allows us to apply these kinds of changes in a backwards compatible fashion.

For example, rather than just checking for branches, we should use reproducible metadata to determine which branches were created by Netlify CMS, which also gives us the ability to change how we're naming branches. Still haven't thought through some of the challenges of this approach, like the "reproducible" bit, meaning metadata can always be recreated from a repo if the metadata is deleted for some reason.

Note that "metadata" here is referring to one or more files in a branch or ref, which we use to keep track of things.

richtera · 2018-08-07T10:30:03Z

Rebased to current master.

tech4him1 · 2018-08-07T15:10:34Z

@richtera To be clear, we do really appreciate your work here, even through we're looking at other options as well!

richtera · 2018-08-07T15:44:22Z

Using metadata to assign destination branches would be great. I don't know enough about the internals to pull that off myself. Ideally then in the workflow pane you might be able to choose the working branch to peek at other changes and maybe pull them but it might add a lot of complications.

richtera · 2018-08-21T19:27:52Z

@tech4him1 Other options would be fine, but currently the new setup makes it pretty much impossible to roll out my patch (because of the lerna repo.) Essentially my requirement is that each netlify deployment (with cms) linked to a branch of a repo should have a standalone workflow. Not doing this was causing changes to be lost all the time because publishing on the wrong branch would move the pull request to the other branch. How about just configuring the cms branch prefix and not supporting multiple ones since each branch has it's own config.yml anyways? Would that be more acceptable?

Benaiah · 2018-08-21T21:41:50Z

Allowing the CMS branch prefix to be customized in the config.yml makes sense to me - we should allow implementors to work around branch name conflicts regardless of the underlying cause. Even with proper metadata in place like @erquhart suggests, we'll likely still want to use a cms-specific prefix for any branches created by the CMS, so it's worth allowing that branch name to be customized if, for whatever reason, it conflicts. I do think that backend.branch_prefix makes more sense than backend.cms_branch_prefix.

@tech4him1 @erquhart thoughts? Is there a reason we shouldn't make this configurable? We can't reasonably guarantee that cms/<anything> will never be a used branch name in git repos, especially those used for storing content.

Benaiah

This approach makes sense to me - @richtera I can write and push the fixes to my review comments to your branch if you'd prefer.

Benaiah · 2018-08-21T21:36:30Z

packages/netlify-cms-backend-github/src/implementation.js

+            sem.leave();
+            resolve(null);
+          }));
+        }));


It looks like something's been truncated here, since this is a syntax error and the function no longer returns anything, and the PR as a whole needs to be formatted by running yarn run format:prettier --write.

Checking. This was a merge conflict in the branch which I tried to resolve on github. Bad idea.

Benaiah · 2018-08-21T21:43:53Z

packages/netlify-cms-backend-github/src/API.js

-    this.repoURL = `/repos/${this.repo}`;
-    this.merge_method = config.squash_merges ? 'squash' : 'merge';
+    this.branch = config.branch || "master";
+    if (config.cms_branch_prefix) {


The config argument here is not the value of the config.yml, it's the argument passed in here: https://github.com/netlify/netlify-cms/blob/master/packages/netlify-cms-backend-github/src/implementation.js#L45-L52. Thus, this isn't actually going to work, though it's very close.

Additionally, I would suggest we use backend.branch_prefix instead of just cms_branch_prefix at the top level of the config.

Yes I see. I need to fix that I had passed in something else. Fixing.
I like "branch_prefix" as the name. Changing.

Benaiah · 2018-08-21T21:44:26Z

website/content/docs/beta-features.md

+  cms_branch_prefix: cms-dev
+```
+
+The default for this value is "cms"


Docs need to be cleaned up before merge.

I can clean it up, but I am not sure what cleaning is required?

Most of the content of the docs here is in the heading, which should just be a short title such as "Custom Branch Name Prefix". The content in the heading should be moved to a short explanatory paragraph, and the 2nd-person voice ("You can configure...", "To enable this feature, you can set...") from earlier in the document should be maintained. A quick note of the reason you might use this (i.e., branch name collisions) would be good too. Thanks!

erquhart · 2018-08-21T22:54:02Z

I'd like to find a way to use the PR base branch, just leaves the problem of determining which PRs were created by the CMS.

_{Sent with GitHawk}

richtera · 2018-08-21T23:20:33Z

@erquhart I think no matter what you'd need different branch prefixes for multiple source branches since branch names need to be unique and editing the same slug in both branches would still cause a conflict, but maybe there is a workaround there.

erquhart · 2018-08-21T23:30:46Z

We could use some kind of shortened unique id. A timestamp could even work for our purposes. I'll give this a think tomorrow.

_{Sent with GitHawk}

erquhart · 2018-08-28T19:56:11Z

I think this is the next step: #1669

erquhart · 2018-08-29T15:24:18Z

Gave this another look and it really is a simple enough change. @Benaiah I think I missed your comment on the first pass, agreed.

I do think branch/branch_prefix is going to cause confusion - they look related but they're not at all. We should do worflow_branch_prefix.

@richtera if you can push that change I'll get this merged.

richtera · 2018-08-30T00:04:41Z

@erquhart I still says changes are requested but as far as I know those were changed. Not quite sure how to make sure all the right things have been committed. I simplified this to branch_prefix inside of backend

erquhart · 2018-08-30T00:16:24Z

It needs to be workflow_branch_prefix, I explained a bit of the reasoning in my last comment.

_{Sent with GitHawk}

richtera · 2018-08-30T00:27:20Z

Ah perfect. I will do it tomorrow morning. The review links always refer to really old stuff for some reason :)

erquhart · 2018-08-31T17:25:20Z

@Benaiah I pushed another commit, please sanity check and merge if good.

richtera · 2018-09-01T23:38:53Z

Ah cool. I overlooked the one change I like removing the / from the setting

This reverts commit da0f520.

richtera mentioned this pull request Jul 10, 2018

Serious problems when using multiple branches. #1493

Closed

erquhart suggested changes Jul 31, 2018

View reviewed changes

richtera force-pushed the master branch from 11f64f4 to 6e54122 Compare August 1, 2018 07:44

tech4him1 requested a review from erquhart August 3, 2018 14:57

richtera force-pushed the master branch from 2dc60e6 to be84912 Compare August 7, 2018 10:29

Benaiah suggested changes Aug 21, 2018

View reviewed changes

Add branch_prefix configuration to backend.

d062ce8

richtera force-pushed the master branch from da9fc27 to d062ce8 Compare August 29, 2018 16:02

Change name of branch_prefix to workflow_branch_prefix

5b8025a

Benaiah approved these changes Aug 30, 2018

View reviewed changes

improvements

16649be

erquhart approved these changes Aug 31, 2018

View reviewed changes

erquhart added 2 commits September 4, 2018 11:43

simplify docs

4bb9049

fix bug

3691a3a

erquhart merged commit da0f520 into decaporg:master Sep 4, 2018

erquhart added a commit that referenced this pull request Sep 17, 2018

Revert "improvement: allow custom workflow branch name prefix (#1494)"

a28ed72

This reverts commit da0f520.

Fix multi branch github deployes by appending branch name for cms pul… #1494

Fix multi branch github deployes by appending branch name for cms pul… #1494

Conversation

richtera commented Jul 10, 2018

verythorough commented Jul 10, 2018 • edited Loading

verythorough commented Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richtera commented Aug 1, 2018

richtera commented Aug 1, 2018

richtera commented Aug 1, 2018

erquhart commented Aug 1, 2018 • edited Loading

richtera commented Aug 2, 2018 • edited Loading

tech4him1 commented Aug 2, 2018

richtera commented Aug 2, 2018

richtera commented Aug 3, 2018

erquhart commented Aug 4, 2018 • edited Loading

richtera commented Aug 7, 2018

tech4him1 commented Aug 7, 2018 • edited Loading

richtera commented Aug 7, 2018

richtera commented Aug 21, 2018 • edited Loading

Benaiah commented Aug 21, 2018

Benaiah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erquhart commented Aug 21, 2018

richtera commented Aug 21, 2018

erquhart commented Aug 21, 2018 • edited Loading

erquhart commented Aug 28, 2018

erquhart commented Aug 29, 2018

richtera commented Aug 30, 2018

erquhart commented Aug 30, 2018

richtera commented Aug 30, 2018

erquhart commented Aug 31, 2018 • edited Loading

richtera commented Sep 1, 2018

verythorough commented Jul 10, 2018 •

edited

Loading

verythorough commented Jul 10, 2018 •

edited

Loading

erquhart commented Aug 1, 2018 •

edited

Loading

richtera commented Aug 2, 2018 •

edited

Loading

erquhart commented Aug 4, 2018 •

edited

Loading

tech4him1 commented Aug 7, 2018 •

edited

Loading

richtera commented Aug 21, 2018 •

edited

Loading

erquhart commented Aug 21, 2018 •

edited

Loading

erquhart commented Aug 31, 2018 •

edited

Loading