Add branch name max length option to GitHub backend (closes #526) #862

Benaiah · 2017-12-05T00:10:38Z

- Summary

Adds branch name max length option to GitHub backend. Closes #526.

- Test plan

Tested manually.

- Description for the changelog

Add branch name max length option to GitHub backend (closes Long post titles can lead to invalid hostnames in Netlify deploy previews #526)

verythorough · 2017-12-05T00:13:05Z

Deploy preview ready!

Built with commit 45b1598

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

tech4him1 · 2017-12-05T00:33:03Z

src/backends/github/API.js

+  truncateSlugForBranchName(slug) {
+    const slugMaxLength = this.branchNameMaxLength - CMS_BRANCH_PREFIX.length;
+    return slug.length > slugMaxLength
+      ? slug.substring(0, slugMaxLength)


Can we just take out the ternary here? If the string is smaller than it, shouldn't the string just be left as-is? I'm not experienced enough with JS string behavior to know whether that is the case or not, but it seems to be.

Benaiah · 2017-12-05T20:58:40Z

@tech4him1 addressed your comment, please re-review.

tech4him1 · 2017-12-05T22:55:26Z

src/backends/github/API.js

@@ -15,6 +15,7 @@ export default class API {
    this.branch = config.branch || "master";
    this.repo = config.repo || "";
    this.repoURL = `/repos/${ this.repo }`;
+    this.branchNameMaxLength = (config.backend && config.backend.branchNameMaxLength) || 40;


Did you actually test this line? I'm not sure how it works, seeing that config is not the actual config.yml, only the values that are passed in here: https://github.com/netlify/netlify-cms/blob/ca1ecc5fc04afdaad4726c2d0eb71e402f6e19a9/src/backends/github/implementation.js#L33

Yeah we need to start rooting out ambiguous variable names like this, there's a lot of it in our source.

verythorough · 2017-12-19T18:33:53Z

Deploy preview ready!

Built with commit 45b1598

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

tech4him1 · 2018-03-27T16:11:59Z

@Benaiah Sorry for the delay. I'm seeing two things right now, one needs to be handled and one may not be:

If the branch_name_max_length is less than 5, we need to throw an error at config load point, since it always must be at least 5 chars to account for cms/.

If the branch_name_max_length is reduced after a post draft is created, the post will no longer show up in editorial workflow if it's slug had to be trimmed. I'm not sure if we can fix this, or just have a warning in the documentation.

erquhart · 2018-03-28T13:43:16Z

I would say we aren't set up for branch names to change, so updating that config should only affect new branches and not existing branches.

tech4him1 · 2018-04-19T14:01:10Z

@erquhart I agree, do you have any suggestions on how that could be implemented?

erquhart · 2018-04-19T21:24:34Z

Only reference/enforce it when creating branches.

tech4him1 · 2018-04-20T03:11:38Z

@erquhart I don't see how that works. If we save the metadata to a truncated name, how do we know what to get it back with without truncating? For example, if the slug is long-filename, and truncated it is long, how do we know what to get it as when attempting to read metadata for long-filename (assuming that the truncation length is different)?

erquhart · 2018-04-20T03:54:47Z

I may be under thinking this, but I'd expect we'd set the same name for metadata and branch?

_{Sent with GitHawk}

tech4him1 · 2018-04-20T13:37:40Z

Well, that's not really what I'm trying to point out, but I could be wrong. If we call retrieveMetadata with a slug (from updateUnpublishedEntryStatus for example), isn't that going to be the full path, not the truncated one? We have to truncate it to get the metadata or branch name, but to do that we have to assume the truncation length hasn't changed.

erquhart · 2018-04-20T14:33:57Z

Hmm we'd need to find a way to avoid truncating for read purposes, maybe through changing the metadata we track so the branch name itself is recorded.

_{Sent with GitHawk}

tech4him1 · 2018-04-20T14:42:02Z

That's the thing -- this PR currently truncates both the metadata filename and the branch name.

erquhart · 2018-04-20T15:55:23Z

Ah, understood! We shouldn't do that, agreed.

tech4him1 · 2018-04-20T17:46:29Z

Also, do you still think the best way to track the branch name is by saving in the metadata, or do you have other suggestions here?

erquhart · 2018-04-24T15:18:20Z

Haven't dug into it very deep, but I can't think of a better way at the moment. You?

erquhart · 2018-07-31T21:21:44Z

Closing as stale.

tech4him1 reviewed Dec 5, 2017

View reviewed changes

Benaiah force-pushed the branch-name-max-length branch from c8d10cf to ca1ecc5 Compare December 5, 2017 20:58

tech4him1 suggested changes Dec 5, 2017

View reviewed changes

Add branch name max length option to GitHub backend (closes #526)

45b1598

Benaiah force-pushed the branch-name-max-length branch from ca1ecc5 to 45b1598 Compare December 19, 2017 18:24

tech4him1 requested review from tech4him1 and erquhart January 29, 2018 22:31

erquhart added impact: docs impact: changelog area: configuration impact: configuration area: extensions/backends impact: example project labels Feb 1, 2018

tech4him1 removed request for erquhart and tech4him1 March 27, 2018 16:13

erquhart closed this Jul 31, 2018

tech4him1 added the status: stale label Aug 7, 2018

erezrokah deleted the branch-name-max-length branch February 7, 2021 15:37

snyk-bot mentioned this pull request Dec 6, 2022

[Snyk] Security upgrade simple-git from 2.48.0 to 3.15.0 B3EF/netlify-cms#28

Open

B3EF mentioned this pull request Jan 24, 2023

[Snyk] Security upgrade simple-git from 2.48.0 to 3.16.0 B3EF/netlify-cms#32

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add branch name max length option to GitHub backend (closes #526) #862

Add branch name max length option to GitHub backend (closes #526) #862

Benaiah commented Dec 5, 2017

verythorough commented Dec 5, 2017 •

edited

Loading

tech4him1 Dec 5, 2017

Benaiah commented Dec 5, 2017

tech4him1 Dec 5, 2017

erquhart Dec 6, 2017

verythorough commented Dec 19, 2017

tech4him1 commented Mar 27, 2018 •

edited

Loading

erquhart commented Mar 28, 2018

tech4him1 commented Apr 19, 2018

erquhart commented Apr 19, 2018

tech4him1 commented Apr 20, 2018

erquhart commented Apr 20, 2018

tech4him1 commented Apr 20, 2018 •

edited

Loading

erquhart commented Apr 20, 2018

tech4him1 commented Apr 20, 2018

erquhart commented Apr 20, 2018

tech4him1 commented Apr 20, 2018 •

edited

Loading

erquhart commented Apr 24, 2018

erquhart commented Jul 31, 2018

Add branch name max length option to GitHub backend (closes #526) #862

Add branch name max length option to GitHub backend (closes #526) #862

Conversation

Benaiah commented Dec 5, 2017

verythorough commented Dec 5, 2017 • edited Loading

tech4him1 Dec 5, 2017

Choose a reason for hiding this comment

Benaiah commented Dec 5, 2017

tech4him1 Dec 5, 2017

Choose a reason for hiding this comment

erquhart Dec 6, 2017

Choose a reason for hiding this comment

verythorough commented Dec 19, 2017

tech4him1 commented Mar 27, 2018 • edited Loading

erquhart commented Mar 28, 2018

tech4him1 commented Apr 19, 2018

erquhart commented Apr 19, 2018

tech4him1 commented Apr 20, 2018

erquhart commented Apr 20, 2018

tech4him1 commented Apr 20, 2018 • edited Loading

erquhart commented Apr 20, 2018

tech4him1 commented Apr 20, 2018

erquhart commented Apr 20, 2018

tech4him1 commented Apr 20, 2018 • edited Loading

erquhart commented Apr 24, 2018

erquhart commented Jul 31, 2018

verythorough commented Dec 5, 2017 •

edited

Loading

tech4him1 commented Mar 27, 2018 •

edited

Loading

tech4him1 commented Apr 20, 2018 •

edited

Loading

tech4him1 commented Apr 20, 2018 •

edited

Loading