-
Notifications
You must be signed in to change notification settings - Fork 73
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
How to resolve compliance violations #1264
Conversation
docs/contributing.md
Outdated
|
||
There are CI Jobs which are setup to monitor the compliance violations in the SDK Generator repositories. For the JS SDK Repository, the CI Job could be found [here](https://dev.azure.com/azure-sdk/internal/_componentGovernance/101977/alert/5809940?typeId=7613604). Now, these violations should be resolved permanently. | ||
|
||
Such dependencies are added to the `package.json` file in the [`packageFileGenerator.ts`](https://github.com/Azure/autorest.typescript/blob/main/src/generators/static/packageFileGenerator.ts). This file has 2 methods `restLevelPackage` & `regularAutorestPackage`. Determine which package is causing the violation and make the appropriate changes. For example, let us say `@opentelemetry/api` package of version `0.10.2` is causing a compliance violation. The solution is to upgrade the version to `1.0.3`. (Usually, such recommendations are found in the compliance issue itself) So, change the version and create a PR. Once the PR is merged & released, any and all SDKs generated with the new version will not have the compliance issue. |
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.
Would it be worth mentioning that this change would require an npm run build && npm run generate-swaggers && npm run smoke-tests
to regenerate all test swaggers?
docs/contributing.md
Outdated
@@ -179,3 +179,12 @@ To debug code generation the flag –typescript.debugger can be used | |||
- Make sure to re-build autorest.typescript after any changes you want to test | |||
|
|||
npm run build | |||
|
|||
### I've got compliance violations for the generated SDKs. How do I resolve it? |
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.
I would recommend having this section talk about dependency updates in general and not in the context of compliance violations. In the azure-sdk-for-js repo we now have automation looking for outdated dependencies and logging issues with label dependency-upgrade-required. If any of these are part of the generated code, then we will be opening issues in the autorest.typescript repo to update the dependency in the generated code. I would like to add help-wanted label on such issues in the code gen repo and point to the contributing guide as to how to go about making the change.
So, think of a section like `How to update dependencies in generated package.json files?".
@@ -1,6 +1,6 @@ | |||
The MIT License (MIT) | |||
|
|||
Copyright (c) 2021 Microsoft | |||
Copyright (c) 2022 Microsoft |
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 are these changes in this PR? Can we move it to another PR instead?
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.
I also have these changes on a PR I opened today, looks like the first PR of the year will have this when regenerating the tests swaggers.
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.
This is the result of regeneration.
b8a1e21
to
a3203d5
Compare
This Change is to add instructions in the contributing document to resolve compliance violations reported in the JS SDK Repository.
@joheredi @ramya-rao-a Please review and approve the PR