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

[Search] - Update search-documents to the latest core packages #17393

Conversation

maorleger
Copy link
Member

@maorleger maorleger commented Sep 1, 2021

As we've been doing for other packages that rely on core-tracing, this hotfix will bring @azure/search-documents to [email protected], ES2017, and [email protected]

Normally we do a minor version bump for such upgrades, but since we have two betas on 11.3.0 already we decided to use a
patch here.

This will resolve items such as #17303

sdk/search/search-documents/CHANGELOG.md Outdated Show resolved Hide resolved
sdk/search/search-documents/CHANGELOG.md Show resolved Hide resolved

- Support for Node.js 8 and IE 11 has been dropped. Please see our [support policy](https://github.com/Azure/azure-sdk-for-js/blob/main/SUPPORT.md) for more details.
- Changed TS compilation target to ES2017 to produce smaller bundles and use more native platform features.
- Updated our internal core package dependencies to their latest versions to add support for Opentelemetry 1.0.0, which is compatible with the latest versions of our other client libraries.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow! Am surprised there are no code changes accompanying this. Is that because search does not have tests for tracing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Most packages do not need code changes at the package level. This is really only an issue because we call Span#context in tracingPolicy which unfortunately is the method they decided to rename...

Copy link
Member Author

Choose a reason for hiding this comment

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

With that said, there is one build that is failing - so maybe code changes will be coming 😄 ...

I still think no code changes will be needed, as the rest of the builds are passing for this hotfix

{
"extends": ["../../.eslintrc.json"],
"rules": {
"@azure/azure-sdk/ts-package-json-engine-is-present": "warn"
Copy link
Member Author

Choose a reason for hiding this comment

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

The codebase at this point in time required engines.node to be set to >= 8.0.0, and I didn't want to start shaving yaks by changing the rule. So I think warn is ok, but let me know if you disagree

Copy link
Contributor

@ramya-rao-a ramya-rao-a Sep 2, 2021

Choose a reason for hiding this comment

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

This is fine by me. Alternatively we can skip the change to the engines field. The next 11.3.0 will have that update anyway and we are doing our due diligence in the readme file around documenting the re-requisites

@@ -536,11 +536,6 @@
"projectFolder": "sdk/eventhub/mock-hub",
"versionPolicyName": "utility"
},
{
Copy link
Member Author

Choose a reason for hiding this comment

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

This hotfix has nothing to do with identity, but this codebase does not contain the necessary changes to support node 16 tests and identity will continue to fail here. I don't think backporting it to support tests for a package I am not releasing (and that search-documents doesn't depend on) is the better approach so I will remove it from the rush.json as we've done in other hotfix branches

@maorleger
Copy link
Member Author

@sarangan12 - I have Ramya's approval, but would love yours as well. Can I get your 👍 on this? I'd love to merge this in by 1pm PDT if possible. Thanks so much!

@sarangan12 sarangan12 merged commit a23a118 into hotfix/bump-core-tracing-search-documents Sep 3, 2021
@sarangan12 sarangan12 deleted the update-core-tracing-for-search-docs branch September 3, 2021 17:38
@ramya-rao-a
Copy link
Contributor

Thanks @maorleger!

@sarangan12 Lets release this hotfix along with the planned release for 11.3.0 beta next week

@sarangan12
Copy link
Member

@ramya-rao-a Sure.

openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Feb 10, 2022
Review request for Microsoft.ContainerService to add version 2022-01-02-preview (Azure#17579)

* Adds base for updating Microsoft.ContainerService from version preview/2021-11-01-preview to version 2022-01-02-preview

* Updates readme

* Updates API version in new specs and examples

* Add NetworkPlugin none option (Azure#17393)

* Update readme to help generate SDK (Azure#17372)

* update readme to help generate SDK

* update readme

* fix golang readme

* fix tag

* Add adminUsers in aadprofile (Azure#17367)

* Add  adminUsers in aadprofile

* Update description

* Support query parameter 'format' in listClusterUserCredential handler (Azure#17211)

* Support query parameter 'format' in listClusterUserCredential handler

* Fix lint

* Fix typo

* Add property HostGroupID and its examples (Azure#17459)

* Add property HostGroupID and its examples

* fix format issue

* add more details for Dedicated host group

Co-authored-by: Jianping Zeng <[email protected]>

* Revert "Add adminUsers in aadprofile (Azure#17367)" (Azure#17522)

This reverts commit ba6e6563690b800d7bda463baecde152fe088884.

* fix conflict (Azure#17624)

Co-authored-by: Matt Stam <[email protected]>
Co-authored-by: Tongyao Si <[email protected]>
Co-authored-by: Jianping Zeng <[email protected]>
Co-authored-by: Jianping Zeng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants