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

[core] core-http and core-client should re-export core-tracing interfaces #16872

Closed
wants to merge 14 commits into from

Conversation

maorleger
Copy link
Member

@maorleger maorleger commented Aug 11, 2021

It's super early draft so I will of course clean things up before taking it out of draft but I wanted to spike out what things would look like if we don't have packages depend on core-tracing directly, instead getting the interfaces they need transitively through core-*

Where needed for tests we can always take a devDependency on core-tracing but at that point it's tech debt instead of potential source of incompat bugs...

sdk/core/core-client/review/core-client.api.md Outdated Show resolved Hide resolved
sdk/core/core-client/src/interfaces.ts Outdated Show resolved Hide resolved
import { Debugger } from '@azure/logger';
import { GetTokenOptions } from '@azure/core-auth';
import { isTokenCredential } from '@azure/core-auth';
import { OperationTracingOptions } from '@azure/core-tracing';
import { Span } from '@azure/core-tracing';
import { CreateSpanFunctionArgs as SpanConfig } from '@azure/core-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.

These used to be named SpanConfig. Richard made them compatible with CreateSpanFunctionArgs so I was thinking exporting both in case someone is using the named type?

Copy link
Member

Choose a reason for hiding this comment

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

I'm less fussed about losing interface types for arguments, but as a sop to compat it could still be aliased as deprecated I suppose.

sdk/core/core-http/review/core-http.api.md Outdated Show resolved Hide resolved
@maorleger maorleger requested a review from xirzec August 11, 2021 22:25
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Overall I like this approach and I think reducing the spread of dependencies feels better. Left some small remarks

common/config/rush/common-versions.json Outdated Show resolved Hide resolved
sdk/core/core-client/review/core-client.api.md Outdated Show resolved Hide resolved
sdk/core/core-client/review/core-client.api.md Outdated Show resolved Hide resolved
sdk/core/core-client/src/interfaces.ts Outdated Show resolved Hide resolved
import { Debugger } from '@azure/logger';
import { GetTokenOptions } from '@azure/core-auth';
import { isTokenCredential } from '@azure/core-auth';
import { OperationTracingOptions } from '@azure/core-tracing';
import { Span } from '@azure/core-tracing';
import { CreateSpanFunctionArgs as SpanConfig } from '@azure/core-tracing';
Copy link
Member

Choose a reason for hiding this comment

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

I'm less fussed about losing interface types for arguments, but as a sop to compat it could still be aliased as deprecated I suppose.

sdk/keyvault/keyvault-admin/package.json Show resolved Hide resolved
@maorleger maorleger requested a review from chradek August 11, 2021 23:09
@check-enforcer
Copy link

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

@maorleger maorleger force-pushed the core-http-exports-all-things branch from 9610120 to 003395c Compare August 13, 2021 16:42
"DeviceUpdateClient-getGroup",
coreHttp.operationOptionsToRequestOptionsBase(options || {})
);
const { span, updatedOptions } = createSpan("DeviceUpdateClient-getGroup", options);
Copy link
Member Author

@maorleger maorleger Aug 13, 2021

Choose a reason for hiding this comment

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

Without this I was getting:

src/operations/devices.ts(1101,7): error TS2559: Type 'RequestOptionsBase' has no properties in common with type '{ tracingOptions?: OperationTracingOptions | undefined; }'.

Which I have to still figure out why the sudden difference and how it ever worked before...

@maorleger maorleger changed the title Re-export all the things from core-tracing [core] core-http and core-client should re-export core-tracing interfaces Aug 13, 2021
@maorleger
Copy link
Member Author

Going to close this, I was thinking it'll be really hard to keep this policy in place and we are better off investing in GAing core-tracing

@maorleger maorleger closed this Sep 20, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Dec 9, 2021
Microsoft.SecurityInsights 2021-09-01-preview (Azure#16933)

* Adds base for updating Microsoft.SecurityInsights from version preview/2021-03-01-preview to version 2021-09-01-preview

* Updates readme

* Updates API version in new specs and examples

* Microsoft.security insights 2021 09 01 preview add missing resources (Azure#15531)

* Copy missing resources specs and examples from 2019-01-01-preview

* Update added resources specs and examples and extract common types

* Update readme

* Extract ClientInfo, UserInfo and Lable to common types

* Fix SpellCheck and Avocado

* Return ThreatIntelligence to readme

* Fix broken refs in Watchlists

* Resolve duplicate schema errors

* Run prettier

* Make common types prettier

* Add required property to operations according to ARM requirments

* Fix readme

* Add file separators to readme

* Rename example file

* Supress OBJECT_ADDITIONAL_PROPERTIES

* Add 'where' to OBJECT_ADDITIONAL_PROPERTIES supression

* Move OBJECT_ADDITIONAL_PROPERTIES supression under general Supression section.

* Copy dataConnectors from 2021-03-01-preview

* Update version of dataConnectors (this was done as there were errors when trying to generate C# client. Copying and changing version again fixed the problem).

* Add dataConnectorsCheckRequirments path, parameters and definitions from 2019-01-01-preveiw

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Use newest common types in new 2021-09-01-preview API version (Azure#15778)

* Use newest common types in AlertRules

* Use newest common types in AutomationRules

* Use newest common types in Bookmarks

* Use newest common types in dataConnectors

* Use newest common types in Enrichment

* Use newest common types in Entities

* Use newest common types in EntityQueries

* Use newest common types in Incidents

* Use newest common types in Metadata

* Use newest common types in OfficeConsents

* Use newest common types in OnboardingStates

* Use newest common types in operations

* Use newest common types in Settings

* Use newest common types in SourceControls

* Use newest common types in ThreatIntelligence

* Use newest common types in Watchlist

* Use newest common types in EntityTypes

* Use newest common types in RelationTypes

* Fix ThreatIntelligence

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Add template version to the scheduled alert rule + scheduled template (Azure#15919)

* Add template version to the scheduled alert rule

* Update AlertRules.json

* Update AlertRules.json

* Update AlertRules.json

* Update AlertRules.json

* Update GetAlertRuleTemplates.json

* Update GetAlertRuleTemplateById.json

* add aws s3 connector (Azure#15844)

* Add a new kind of alert rules - NRT (Azure#15980)

* add NRT rule

* add NRT rule

* add NRT rule

* add NRT rule

* fix typo

* fix typo

* fix

* Align new Metadata feature with 2021-03-01-preview (Azure#16304)

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Add fixes from 2021-03-01-preview (Azure#16238)

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Add entity query templates (Azure#16269)

* Add entity query templates from 2021-03-01-preview

* Update version

* Use newest common types and update readme

* Fix conflicting common types

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Fix bookmark relations operatinIds to be consistent with other operationIds. (Azure#16519)

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Add corrections from 2021-03-01-preview (Azure#16490)

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Remove unused parameters (Azure#16619)

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Update readme default readme tag for client generation (Azure#16620)

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Use CloudError instead of ErrorResponse to avoid breaking change (Azure#16691)

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Add data connectors polling ccp api support (Azure#16293)

* adding dataConnectors polling CCP api Support. (witout tests validations)

* azure sentinel dataconnectors update examples

* azure sentinel dataConnectors examples update and fix

* azure sentinel dataConnectors prettier

* azure sentinel dataConnectors add connect disconnect examples update path

* azure sentinel dataConnectors add connect disconnect examples fix

* azure sentinel dataConnectors add connect disconnect examples fix 2

* azure sentinel dataConnectors rebase dataConnectors dev

* azure sentinel dataconnectors - fix put to post on connect and disconnect endpoints

* azure sentinel dataconnectors - adding x-ms-secret to password on connect

* azure sentinel dataconnectors - connect/disconnect endpoint remove unnedded 201 return

* azure sentinel dataConnectors - remove empty body DataConnectorDisconnectBody

Co-authored-by: Alon Danoch <adanoch@microsoft.com>

* Add office IRM Connector (Azure#16764)

* Add office IRM

* fix

* fix

* fix

* fix

Co-authored-by: omerhaimov <omer.haimovich@gmail.com>

* Add teamInformation to IncidentProperties 2021-09-01-preview (Azure#16787)

* Fix Swagger for SecurityInsights - Add teamInformation to IncidentProperties

* Try change description as advised by Swagger reviewer Yuchao Yan to fix the validation error.

* Revert change in ntDomain description as it has nothing to do with validations

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Make CloudError and CloudErrorBody external resources (already exist under Microsoft.Rest.Azure namespace) (Azure#16872)

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Remove operational insights parameter 2021 09 01 preview (Azure#16891)

* Remove operationalInsightsResourceProvider parameter from specs

* Remove parameter from examples

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>

* Update EntityTypes.json (Azure#16972)

Co-authored-by: Anat Gilenson <anatgilenson@microsoft.com>
Co-authored-by: Amit Bergman <38046493+Amitbergman@users.noreply.github.com>
Co-authored-by: sagamzu <52034287+sagamzu@users.noreply.github.com>
Co-authored-by: necoh <53861229+necoh@users.noreply.github.com>
Co-authored-by: alondanoch <alondanoch@hotmail.com>
Co-authored-by: Alon Danoch <adanoch@microsoft.com>
Co-authored-by: omerhaimov <55688621+omerhaimov@users.noreply.github.com>
Co-authored-by: omerhaimov <omer.haimovich@gmail.com>
Co-authored-by: Yuchao Yan <yuchaoyan@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants