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

Upgrade autorest #2370

Merged
merged 22 commits into from
Aug 16, 2023
Merged

Upgrade autorest #2370

merged 22 commits into from
Aug 16, 2023

Conversation

DmitriyKirakosyan
Copy link
Contributor

@DmitriyKirakosyan DmitriyKirakosyan commented Jul 4, 2023

PR Summary

This PR presents a significant update to the codebase, primarily focusing on upgrading the autorest package to version 3, phasing out the deprecated and security risk-laden request dependency, and accommodating for the breaking changes including the shift from a callback-based API to a promise-based one.

Changes Implemented:

  1. Removed request: All code has been updated to use fetch instead of request. I specifically chose to use node-fetch over alternatives such as got or axios primarily to avoid introducing additional dependencies and their subsequent cascading dependencies. Notably, node-fetch was already part of our existing dependencies.
  2. Migrated ms-rest to ms-rest-js: As the ms-rest package listed request as a dependency, it has been migrated to ms-rest-js.
  3. Upgraded autorest to version 3: The earlier used autorest version 2 utilized ms-rest, hence it was upgraded to version 3.
  4. Updated Swagger API file: It's made compatible with the new autorest version.
  5. Regenerated API files: The API files were regenerated using the updated autorest and Swagger API file.
  6. Modified create-client class: Adjustments were made to build and use the new service client effectively.
  7. Refactored Commands: All affected commands in the application have been modified according to the new API generated from the files.
  8. Resolved Compilation Issues: All compilation errors arising due to these changes have been fixed.
  9. Updated Tests: Tests have been updated to align with the new changes.

Reviewing This PR

Note: All files within "src/util/apis/generated/src" were auto-generated by autorest using the "scripts/autorest.js" script and can be ignored during the review.

  1. Begin with create-client class: This class is the foundation as it instantiates and configures the App Center client service used for API calls.
  2. Review Changed Commands: Navigate through the modified commands in "src/commands" and look for any suspicious changes, such as code removal, missing blocks, or improper error handling.
  3. Review Test Modifications: Examine the changes made to the tests.

Optional: If you have additional time, you can fetch and test these changes in your local setup. If you do so, please comment on this PR about the commands you managed to test, to help others focus on untested parts.

Related Work Items:

This PR is related to the following work items:

@DmitriyKirakosyan DmitriyKirakosyan requested a review from a team as a code owner July 4, 2023 14:24
scripts/autorest.js Outdated Show resolved Hide resolved
src/commands/apps/create.ts Outdated Show resolved Hide resolved
src/commands/apps/lib/format-app.ts Outdated Show resolved Hide resolved
src/commands/apps/update.ts Outdated Show resolved Hide resolved
src/commands/build/branches/list.ts Outdated Show resolved Hide resolved
src/commands/distribute/groups/download.ts Show resolved Hide resolved
src/commands/distribute/groups/list.ts Show resolved Hide resolved
src/commands/distribute/groups/update.ts Show resolved Hide resolved
src/commands/distribute/releases/add-destination.ts Outdated Show resolved Hide resolved
Priskorn
Priskorn previously approved these changes Aug 7, 2023
@AnatolyPristensky
Copy link
Contributor

All affected commands have been tested. Approved.

@DmitriyKirakosyan DmitriyKirakosyan merged commit 7b63774 into master Aug 16, 2023
@DmitriyKirakosyan DmitriyKirakosyan deleted the feature/upgrade-autorest branch August 16, 2023 05:37
@AnatolyPristensky AnatolyPristensky mentioned this pull request Feb 26, 2024
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.

5 participants