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

Feat: Align GitHub metadata handling with other backends #3316

Merged
merged 5 commits into from
Feb 24, 2020

Conversation

erezrokah
Copy link
Contributor

@erezrokah erezrokah commented Feb 24, 2020

Replaces #3292
Fixes #1669
Fixes #3303

This time with a fix to the migration code.
Added a test to migrate from 2.9.7 to current and 2.10.24 to current.
The test:

  1. Seeds the repo with some unrelated CMS data and PRs (more than 100 to trigger pagination).
  2. Switches to a specific version.
  3. Creates a bunch of entries.
  4. Switches to the current built version.
  5. Publishes the entries.

const mapped = pullRequests.nodes.map(transformPullRequest);

return ((mapped as unknown) as Octokit.PullsListResponseItem[]).filter(
pr => pr.head.ref.startsWith(`${CMS_BRANCH_PREFIX}/`) && predicate(pr),
Copy link
Contributor Author

@erezrokah erezrokah Feb 24, 2020

Choose a reason for hiding this comment

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

Same as REST API

@@ -355,7 +368,7 @@ export default class API {
const changeTree = await this.updateTree(branchData.sha, [file as TreeFile]);
const { sha } = await this.commit(`Updating “${key}” metadata`, changeTree);
await this.patchRef('meta', '_netlify_cms', sha);
localForage.setItem(`gh.meta.${key}`, {
await localForage.setItem(`gh.meta.${key}`, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a long standing bug causing caching issues that surfaced in the tests

@@ -41,7 +41,7 @@ export const readFile = async (

const content = await fetchContent();
if (key) {
localForage.setItem(key, content);
await localForage.setItem(key, content);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar caching bug

);

return pullRequests.filter(
pr => pr.head.ref.startsWith(`${CMS_BRANCH_PREFIX}/`) && predicate(pr),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erquhart pr.head.ref.startsWith(`${CMS_BRANCH_PREFIX}/`) was missing from the previous PR.

@erezrokah
Copy link
Contributor Author

Looking into why the test is passing locally on my machine and not on CI

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.

Reviewed the diff between the two PR's, LGTM

@erquhart erquhart merged commit 7e0a8ad into master Feb 24, 2020
@erezrokah erezrokah deleted the feat/new_github_metadata branch February 26, 2020 15:59
vladdu pushed a commit to vladdu/netlify-cms that referenced this pull request Jan 26, 2021
…org#3316)

* Revert "Revert "feat(core): Align GitHub metadata handling with other backends (decaporg#3292)""

This reverts commit 5bdd3df.

* fix(backend-github): fix migration code

* test(backend-github): fix test

* test(e2e): shorten wait time

* test(e2e): try and fix test on CI
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.

Fix workflow migrations from 2.9.7 (stub) Improving metadata handling
2 participants