-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[EngSys] Add dev tool check to analyze step
#36824
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
base: main
Are you sure you want to change the base?
Conversation
9658e20 to
5c6ee6c
Compare
|
Look great and nice to see bugs were fixed! We need to update our code generator first. It would be nice if we can undo the package.json formatting changes to make PR review easier. |
| }, | ||
| "autoPublish": true, | ||
| "homepage": "https://github.com/Azure/azure-sdk-for-js/tree/main/sdk/advisor/arm-advisor", | ||
| "homepage": "https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/advisor/arm-advisor/README.md", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We either need to standardize on the homepage format, or relax dev-tool to allow the format that Management packages use. Standardizing is probably better.
| "build": "npm run clean && dev-tool run build-package && dev-tool run extract-api", | ||
| "build:samples": "tsc -p tsconfig.samples.json && dev-tool samples publish -f", | ||
| "check-format": "prettier --list-different --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"samples-dev/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\"", | ||
| "check:ci": "dev-tool check --tag ci", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a corresponding code generator PR to add the two check NPM scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and pls don't forget to apply this in codegen repo. Thanks!
| node eng/tools/ci-runner/index.js check-format $(ChangedServices) -packages "$(ArtifactPackageNames)" | ||
| displayName: "Check Format in Libraries" | ||
| node eng/tools/ci-runner/index.js check:ci $(ChangedServices) -packages "$(ArtifactPackageNames)" | ||
| displayName: "Check Libraries" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a condition in pipeline build that we can check for whether we should run check:release?
| "engines": { | ||
| "node": ">=20.0.0" | ||
| }, | ||
| "engines": { "node": ">=20.0.0" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was Mgmt package.json formatted? it's "format" script just echo.
| import type { CommunicationIdentifier } from "@azure/communication-common"; | ||
|
|
||
| import type { | ||
| Tone} from "../generated/src/models/index.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting here doesn't look right.
| "vitest": "catalog:testing" | ||
| }, | ||
| "homepage": "https://github.com/Azure/azure-sdk-for-js/tree/main/sdk/communication/communication-messages-rest-rest/README.md", | ||
| "homepage": "https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/communication/communication-messages-rest/README.md", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good fix!
Packages impacted by this PR
Issues associated with this PR
#36589
Describe the problem that is addressed by this PR
Replaces format and lint steps with
dev-tool checkChecklists
***NO_CI***