Skip to content

Remove nodegit dependency#6471

Merged
cee-chen merged 6 commits intoelastic:mainfrom
cee-chen:nodegit
Dec 8, 2022
Merged

Remove nodegit dependency#6471
cee-chen merged 6 commits intoelastic:mainfrom
cee-chen:nodegit

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Dec 8, 2022

Summary

This PR is part of our higher-level goal of removing dependencies causing issues for M1/arm64 developers (see #6442).

Our nodegit dependency is only used during our release script. While there are alpha versions of nodegit that purport to support M1, I spiked out removing/replacing the dependency totally with exec versions of git CLI commands, and was able to successful find their corresponding CLI APIs which I found easy enough to replace.

While these changes may not be totally bullet or bug proof, these changes are dev-only and I feel confident enough to move forward with them as-is and fix any issues that arise during the next release.

QA

Release script

  • Run npm run release and confirm it outputs the following message: Unable to release: currently on branch "nodegit", expected "main"

Tokens script

  • Run node scripts/update-token-changelog.js major and confirm it outputs "No i18n token changes found to commit"
  • Open i18ntokens.json and change the first defString to (e.g.) TEST. Save (without Prettier formatting)
  • Run node scripts/update-token-changelog.js major and confirm it outputs "i18n token changes committed"
  • Run git log and confirm you see an update i18ntokens commit
  • Run git reset HEAD^ and inspect the changed files/diff, and confirm i18ntokens_changelog.json looks like this:
  {
    "version": "72.0.0",
    "changes": [
      {
        "token": "euiAccordion.isLoading",
        "changeType": "modified",
        "value": "TEST"
      }
    ]
  },

General checklist

N/A, dev only

- instead use execSync and git CLI to accomplish the same outcomes
+ remove references to nodegit
- it's no longer of use with `nodegit` removed
@cee-chen cee-chen added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Dec 8, 2022
@cee-chen cee-chen requested review from 1Copenut and breehall December 8, 2022 21:25
Copy link
Copy Markdown
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! I tried out the QA steps on a not-an-M1 machine and everything worked as expected.

@kibanamachine
Copy link
Copy Markdown

Preview documentation changes for this PR: https://eui.elastic.co/pr_6471/

- token script now relies on tags being up to date/correct, so we should fetch/prune them first
Copy link
Copy Markdown
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

🐥 Looks good! We checked this together in screen share. Can't wait to run scripts without my M1 yelling at me!

@cee-chen cee-chen enabled auto-merge (squash) December 8, 2022 22:15
@kibanamachine
Copy link
Copy Markdown

Preview documentation changes for this PR: https://eui.elastic.co/pr_6471/

@cee-chen cee-chen merged commit 71e3bfb into elastic:main Dec 8, 2022
@cee-chen cee-chen deleted the nodegit branch December 8, 2022 22:31
@cee-chen cee-chen mentioned this pull request Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants