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

Fleetctl: Update dependencies, improve error handling, ensure compatibility #24845

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

RachelElysia
Copy link
Member

@RachelElysia RachelElysia commented Dec 17, 2024

Issue

Cerra #24268

Description

  • Update all fleetctl npm dependencies
  • Specifically:
    • "axios": "1.7.9",
    • "rimraf": "6.0.1",
    • "tar": "7.4.3"
  • Axios: Update error handling for more explicit error handling if axios error
  • Rimraf: Update sync to be named rimrafSync which is a name change in v4
  • Tar: All code should be compatible between versions
  • Update yarn.lock file using yarn install

Self QA

  • Run node run.js
  • login, perform a few actions, logout

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  • Manual QA must be performed in the three main OSs, macOS, Windows and Linux.

@RachelElysia RachelElysia marked this pull request as ready for review December 17, 2024 21:28
Copy link
Member

@iansltx iansltx left a comment

Choose a reason for hiding this comment

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

This looks good.

To QA this, I think you only need two paths:

  1. Run it and make sure it downloads/runs fleetctl under normal operation
  2. Swap version in package.json for e.g. 13.37.0 (which will 404) and try running (should get an Axios error back)

Copy link
Member

@iansltx iansltx left a comment

Choose a reason for hiding this comment

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

Actually, scratch my previous review. Tried running this (node run.js) and...we can't use modules here. Need to flip stuff back to require calls since the file needs to be run at the top level.

@iansltx
Copy link
Member

iansltx commented Dec 17, 2024

Also need to update the yarn lock file. Probably useful to run npm i and commit the package lockfile as well given that most folks will be installing this via npm.

@RachelElysia
Copy link
Member Author

Thanks @iansltx - just updated the code and successfully installed and ran fleetctl

@RachelElysia RachelElysia dismissed iansltx’s stale review December 18, 2024 14:40

updated for re-review

@RachelElysia RachelElysia merged commit 8888127 into main Dec 18, 2024
6 checks passed
@RachelElysia RachelElysia deleted the 24268-update-fleetctl-deps branch December 18, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants