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

MVP of CLI to file PRs with Package Updates #1128

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

freeqaz
Copy link
Member

@freeqaz freeqaz commented Feb 16, 2023

There are still a few bugs left to shake out here, but the code is 99% of the way there now.

Example PR generated with this command: yarn run dev github-pr replace-package freeqaz/jira_clone --githubToken <SECRET> --old js-yaml@^3.13.1 --new js-yaml@^3.14.0

freeqaz/jira_clone#2

Bugs left:

  • Figure out why packages are marked "extraneous" in the generated lockfile
  • Name the folder where these packages are inserted to be the same as the repo (the package-lock gets a new name currently and it's annoying)

Items left:

  • Call this module from the backend by adding a new Endpoint for it
  • Write the front-end changes to call the endpoint
  • Write some basic unit tests to test this functionality

There are still a few bugs left to shake out here, but the code is 99% of the way there now.

Example PR generated with this command: `yarn run dev github-pr replace-package freeqaz/jira_clone --githubToken <SECRET> --old js-yaml@^3.13.1 --new js-yaml@^3.14.0`

freeqaz/jira_clone#2

Bugs left:
- [ ] Figure out why packages are marked "extraneous" in the generated lockfile
- [ ] Name the folder where these packages are inserted to be the same as the repo (the package-lock gets a new name currently and it's annoying)

Items left:
- [ ] Call this module from the backend by adding a new Endpoint for it
- [ ] Write the front-end changes to call the endpoint
- [ ] Write some basic unit tests to test this functionality
@factoidforrest
Copy link
Contributor

Neat! Seems like arborist is doing all of the work and youre just lining it up so that it can knock em down. We should have done this a while ago! Piece of cake.

Amazing how it seems to support the different lockfile formats without a hitch. Any testing of that?

Copy link
Contributor

@factoidforrest factoidforrest left a comment

Choose a reason for hiding this comment

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

Cool! Amazing! Needs loads of testing, ofc. We have a lot of test fixtures and stuff already in the backend test fixture folder, so you can use the package-lock files from those various fixtures to try it.

If you REALLY want a lot of fixtures, go look at the snyk package tree fixtures. They have like 100 lol, and their tests go through almost every scenario you can imagine. We could port those if needed.

});

// TODO: Figure out why Arborist marks everything as "extraneous" in the generated lockfile.
const node = await tree.arborist.loadVirtual();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this code a duplicate of the above code from replace-package/index?

const { escapedName, rawSpec } = npa(oldPackage);

// TODO: Figure out if this works for `git` packages as well. (It probably doesn't and will require a separate code path)
const nodes = await node.querySelectorAll(`[name=${escapedName}]:semver(${rawSpec})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably just bail out on git packages or URL packages.

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.

2 participants