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(command-lm): merge netlify-lm-plugin to CLI #1489

Merged
merged 50 commits into from
Feb 14, 2021

Conversation

keiko713
Copy link
Contributor

@keiko713 keiko713 commented Oct 29, 2020

- Summary

Closes #483
Closes #514
Closes netlify/netlify-lm-plugin#43
Closes netlify/netlify-credential-helper#46

netlify-lm-plugin is not maintained well currently, and using some deprecated dependency. This PR is to see if we can merge the plugin into the CLI to make it easy to maintain.
Also, I would love to retire the usage of old getAddons/createAddon, so that it'll better integrated with the development env var like NETLIFY_API_URL.

- Test plan

Very much WIP.
This is also a great chance to add tests!!

- Description for the changelog

Merge netlify-lm-plugin (netlify lm command) to CLI

- A picture of a cute animal (not mandatory but encouraged)

image

@keiko713 keiko713 added the type: chore work needed to keep the product and development running smoothly label Oct 29, 2020
@keiko713
Copy link
Contributor Author

keiko713 commented Oct 29, 2020

Currently it is half-working. lm:info is running but somehow lm is not working:

$ bin/run lm              
 ›   Error: command lm not found
    Error: Command failed: netlify lm --help
$ bin/run lm --help
Handle Large Media operations

USAGE
  $ netlify lm

DESCRIPTION
  The lm command will help you manage a large media for the site

EXAMPLE
  netlify lm:info

$ bin/run lm:info  
  ✔ Checking Git version [2.26.0]
  ✔ Checking Git LFS version [2.5.1]
  ✔ Checking Git LFS filters
  ✔ Checking Netlify's Git Credentials version [0.1.9]

(also looks like I got a lot of linting warning)

src/commands/lm/info.js Outdated Show resolved Hide resolved
@erezrokah
Copy link
Contributor

Hi @keiko713, how do we want to move forward with this?

@keiko713
Copy link
Contributor Author

keiko713 commented Dec 4, 2020

thanks for the ping and sorry that I haven't touched this for more than one month...!!! I really would like to get back to this, but at the same time, I'm having hard time finding time for this.
can you please give me, let's say, one more week to see if I can find some time?

@erezrokah
Copy link
Contributor

can you please give me, let's say, one more week to see if I can find some time?

Should we can do that. I can also raise #483 internally to see if we can prioritize it.

@keiko713
Copy link
Contributor Author

current status:

  • migrated all the commands
  • confirmed that super basic paths (lm:info, lm:install, and lm:setup) with platform: macOS 10.15.7 works

TODOs

  • write tests
  • more tests with different platform, as well as more edge cases (e.g. when X is not installed)

@keiko713
Copy link
Contributor Author

keiko713 commented Dec 22, 2020

@erezrokah syncing up with some progress (also no rush, totally fine after 🎄 ):

  • I managed to run some test, but still it's been failing with all other platforms than macos (I would love some guidance of cross-platform testing in general)
  • there is some difficulty with the test in general (will comment more inline)
  • I'd love to get some feedback, the tests are failing but the code should be already in a decent shape

Thanks!

tests/command.lm.test.js Outdated Show resolved Hide resolved
@erezrokah
Copy link
Contributor

erezrokah commented Dec 23, 2020

@keiko713 Thank you so much for following up on this.
I rebased the branch and added a few more commits to do some code cleanup - please let know what you think. I might follow up with some more next year :)

For the failing tests, I had to run it on my Windows VM to get the error, see netlify/netlify-credential-helper#49

That made lm:install work on Windows (I updated the code to install from the PR's branch), but lm:setup is still failing so I need to look into it.

Looks like Linux is failing with Error: Unable to detect SHELL type, make sure the variable is defined in your environment

@erezrokah erezrokah force-pushed the keiko/merge-lm-plugin branch 3 times, most recently from b32349b to 949741a Compare January 12, 2021 15:51
@erezrokah erezrokah force-pushed the keiko/merge-lm-plugin branch 3 times, most recently from 3d42b92 to 3559f57 Compare January 13, 2021 18:12
@erezrokah
Copy link
Contributor

cc @keiko713 tests are passing on Windows (Node.js 10 failure is unrelated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs docs type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
3 participants