[core-util] Update "delay" method in core-util with abort options #23019

minhanh-phan · 2022-08-26T17:23:57Z

Packages impacted by this PR

core-util

Issues associated with this PR

#15930

Describe the problem that is addressed by this PR

Update "delay" in core-util to support abort message
Unduplicate "delay" function in core-http and core-amqp

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Choose the implementation with "options" parameters to keep the flexibility in case we want to add extra functionality in the future

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

#15832

Command used to generate this PR:***(Applicable only to SDK release request PRs)*

Checklists

[x ] Added impacted package name to the issue description
Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
Added a changelog (if necessary)

ghost · 2022-08-26T18:04:50Z

All CLA requirements met.

azure-sdk · 2022-08-30T22:45:45Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-core-amqp
azure-core-http
azure-core-util

HarshaNalluru · 2022-08-31T21:34:24Z

sdk/core/core-util/review/core-util.api.md

+// Warning: (ae-forgotten-export) The symbol "DelayOptions" needs to be exported by the entry point index.d.ts
+//
+// @public (undocumented)
+export function delay<T>(timeInMs: number, options?: DelayOptions): Promise<T | void>;
+
+// @public (undocumented)
+export function delay<T>(timeInMs: number, value: T, options?: DelayOptions): Promise<T | void>;


Why do we have two?

Many calls for the delay method have the value parameter set to undefined. We want to overload the method so that the users do not have to pass in undefined

I looked into this with @minhanh-phan offline, and it looks like the overload with value is not really used anywhere, so just keeping a single signature with no value: T

sdk/core/core-util/review/core-util.api.md

sdk/core/core-util/CHANGELOG.md

HarshaNalluru · 2022-08-31T21:36:27Z

sdk/servicebus/service-bus/test/internal/batchReceiver.spec.ts

-            Constants.defaultOperationTimeoutInMs * 1.5,
-            undefined,
-            undefined,
-            "ondetachednevercalled"


looks like the test did not get translated fully?
"ondetachednevercalled" is missing in the update.

We do not have 'abortSignal' passed in, so the message will not actually be used regardless

Yeah if the abort controller is not passed (test was passing undefined), then the abort error is never going to be thrown, so it makes sense to remove it.

Looking deeper, it's not the error message, it's the 4th arg, value.

So, we should retain it for the proper expectation of the test.

The delay function description looks odd because the value parameter has been deleted and it should not be of string type regardless. I'll look into it.

sdk/core/core-util/test/public/delay.spec.ts

sdk/core/core-amqp/src/util/utils.ts

sdk/core/core-util/package.json

sdk/core/core-util/review/core-util.api.md

jeremymeng · 2022-09-01T18:20:26Z

sdk/core/core-util/CHANGELOG.md

@@ -5,6 +5,7 @@
 ### Features Added

 - Add helper type guards `isDefined`, `isObjectWithProperties`, `objectHasProperty`.
+- Update `delay` method to handle `abortSignal` functionality


I merged a PR yesterday to bump core-util version to 1.1.0. This PR should probably wait until after we release core-util today.

HarshaNalluru

Concerned about the service-bus test changes

sdk/core/core-util/src/delay.ts

check-enforcer · 2022-09-02T22:47:01Z

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

HarshaNalluru · 2022-09-07T22:46:40Z

sdk/core/core-amqp/review/core-amqp.api.md

@@ -368,7 +368,7 @@ export function createSasTokenProvider(data: {
 // @public
 export const defaultCancellableLock: CancellableAsyncLock;

-// @public
+// @public @deprecated


Need not be deprecated anymore, right?

Because this supports the value argument and the core-utils one doesn't.

HarshaNalluru

Looks good!

xirzec

One minor nit but wow!! Looks great

sdk/core/core-util/src/delay.ts

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

ghost · 2022-09-08T23:03:21Z

Hello @minhanh-phan!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Feature/cplat 2023 03 01 (Azure#22885) * copy last version to 2023-03-01 folder * update version reference in the new folder * update versions reference in the new folder * add new readme tag with new version * sync with updates made to 2022-11-01 version * fix generation error * fix duplicate enum name thru c# directive instead. * make all the changes to this version that was made to the last version (2022-11-01) to address modelvalidation errors * update * Revert "update" This reverts commit 08417d3ed412ab63c7ee7bdb712ad756ded03c97. * Added $expand option in ListAllVMs and ListAllVMs in RG (Azure#22800) * Added $expand option in ListAllVMs and ListAllVMs in RG * Update virtualMachine.json * dedicatedHost Resize feature (Azure#23268) * dedicatedHost Resize feature * DedicatedHost Sku renamed to size and resolved lint errors. * reviewer comments * httpMethod fix * Add property for VM (Azure#22882) (Azure#23329) Co-authored-by: payalguptapg <126145083+payalguptapg@users.noreply.github.com> * Add Reapply for VMSS (Azure#22344) * Add Reapply for VMSS * Prettier fix * Update examples * Address review comments 01 * Use typical resourceGroupName parameter. * Address review comments - Rename examples * Remove <br> syntax from many descriptions in CRP swagger files (Azure#23019) * up to computeRPCommon * all but computeRPCommons * computerpcommon * vmss clean * common clean * vmss try n * trying only \n * remove \n as it messes with rest docs * cleanup 2022-11-01 accidents * cleanups 2023 * remove ID from Update objects that do not have ID (Azure#23078) * update * add identifiers --------- Co-authored-by: Theodore Chang <theodore.l.chang@gmail.com> * Origin/feature/cplat 2023 03 01 (Azure#23203) * Update virtualMachine.json Added missing property for Additional properties not allowed: provisionAfterExtensions Json path: $.value[*].properties.provisionAfterExtensions * added missing property for provisionAfterExtensions * add locatoin in VirtualMachineScaleSetVMExtension * fixed issue of x-ms-mutability --------- Co-authored-by: Younghyun Kim <younghyunkim@microsoft.com> * Add securityPostureReference (Azure#23106) * [RestorePoints] Adding Encryption, Source Details, HyperVGeneration and WriteAccelerated for Restore Point (Azure#23303) * Restore Point Encryption Details * Modified descriptions and made DiskRestorePointProperties as input property * Renaming object and adding type * Renaming DiskRestorePointProperties * making DiskRestorePoint.id readOnly * Modifying reference * Running Prettier * Renaming EncryptionType * Adding HyperVGeneration and WriteAcceleratorEnabled (prchin) * Add optional parameter hibernate to vmss deallocate api (Azure#23409) * Add optional parameter hibernate to vmss deallocate api * Fix with prettier * update example for vmss deallocate * Add Spot Related Properties to VMSS PATCH * prettier and lintDiff suppressions * retry lintDiff suppression * lint diff suppression retry * Add managed identities parameters for blobs, add treatFailureAsDeploymentFailure flag for Run Command (Azure#23557) * Add managed identities inputs for script, errorBlob, outputBlob * Add treatFailureAsDeploymentFailure flag * Update proximityPlacementGroup.json (Azure#23556) * Update proximityPlacementGroup.json * Update proximityPlacementGroup.json * Update proximityPlacementGroup.json --------- Co-authored-by: Theodore Chang <theodore.l.chang@gmail.com> Co-authored-by: Kartik Gupta <31189137+Kartik-715@users.noreply.github.com> Co-authored-by: cakarata <47228825+cakarata@users.noreply.github.com> Co-authored-by: payalguptapg <126145083+payalguptapg@users.noreply.github.com> Co-authored-by: Anshul Verma <88476874+AnshulVermaa@users.noreply.github.com> Co-authored-by: Adam Sandor <adsandor@microsoft.com> Co-authored-by: younghyun5756 <102988755+younghyun5756@users.noreply.github.com> Co-authored-by: Younghyun Kim <younghyunkim@microsoft.com> Co-authored-by: krishnak-msft <127885427+krishnak-msft@users.noreply.github.com> Co-authored-by: Linu George <127192938+linugeorgeofficial@users.noreply.github.com> Co-authored-by: vatsan28 <vatsan9228@gmail.com> Co-authored-by: Viv Lingaiah <viv.lingaiah@gmail.com> Co-authored-by: Micah McKittrick <32313503+mimckitt@users.noreply.github.com>

minhanh-phan requested review from xirzec, jeremymeng and deyaaeldeen as code owners August 26, 2022 17:23

ghost added the Azure.Core label Aug 26, 2022

minhanh-phan marked this pull request as draft August 26, 2022 17:24

minhanh-phan marked this pull request as ready for review August 31, 2022 21:19

minhanh-phan requested review from bterlson and HarshaNalluru as code owners August 31, 2022 21:19

HarshaNalluru reviewed Aug 31, 2022

View reviewed changes

sdk/core/core-util/review/core-util.api.md Outdated Show resolved Hide resolved

HarshaNalluru reviewed Aug 31, 2022

View reviewed changes

sdk/core/core-util/CHANGELOG.md Outdated Show resolved Hide resolved

HarshaNalluru reviewed Aug 31, 2022

View reviewed changes

sdk/core/core-util/test/public/delay.spec.ts Outdated Show resolved Hide resolved

jeremymeng reviewed Sep 1, 2022

View reviewed changes

sdk/core/core-amqp/src/util/utils.ts Outdated Show resolved Hide resolved

sdk/core/core-util/package.json Outdated Show resolved Hide resolved

sdk/core/core-util/review/core-util.api.md Outdated Show resolved Hide resolved

jeremymeng reviewed Sep 1, 2022

View reviewed changes

HarshaNalluru requested changes Sep 1, 2022

View reviewed changes

HarshaNalluru reviewed Sep 2, 2022

View reviewed changes

sdk/core/core-util/src/delay.ts Outdated Show resolved Hide resolved

minhanh-phan added 10 commits September 6, 2022 17:59

Update delay with abort options

d9d3c77

[core-util] Update delay method with abortSignal functionality

412ad50

[core-http] Unduplicate delay method

5c43bf1

[core-amqp] Unduplicate delay method

cf61427

Unduplicate delay method in core-http

224e606

[core-http] Replace import delay method from core-util

d1ff2b5

[core-amqp] Import delay method from core-util

20dc0d8

[core-amqp] Import delay method from core-util

5b6d4e4

Update version dependency and markdown files

0b4e302

Fix servicebus delay method

8c85c10

minhanh-phan added 14 commits September 6, 2022 18:05

[servicebus] Update to new delay method

c9e9cc4

[core-amqp] Update format

b1065ed

[core-amqp] update format

9eb62be

[core-amqp] format check

7b7083a

[core-util] include test for abort functionality

f9f4581

format check

e2091ec

[core-amqp] delay wrapper update

8f5036e

update names

907cd48

Update from comments

ee9ae0e

[core-util] sort import alphabetically

49bbac5

[core-amqp] fix import and comment statements

3cf9968

servicebus test update

bcb6e36

core-util documentation update

6c64969

Update Changelog

46c31c1

minhanh-phan force-pushed the delay-fix branch from 43c1821 to 46c31c1 Compare September 6, 2022 18:09

minhanh-phan requested a review from HarshaNalluru September 7, 2022 17:24

HarshaNalluru reviewed Sep 7, 2022

View reviewed changes

minhanh-phan added 2 commits September 8, 2022 15:48

Delete deprecated message

7452321

Update md file

a2c88de

minhanh-phan requested a review from HarshaNalluru September 8, 2022 20:40

joheredi approved these changes Sep 8, 2022

View reviewed changes

HarshaNalluru approved these changes Sep 8, 2022

View reviewed changes

xirzec approved these changes Sep 8, 2022

View reviewed changes

sdk/core/core-util/src/delay.ts Outdated Show resolved Hide resolved

Update on style for coding

1c3764e

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

minhanh-phan added the auto-merge label Sep 8, 2022

Format file

61d5884

ghost merged commit a6e366d into Azure:main Sep 9, 2022

minhanh-phan deleted the delay-fix branch March 15, 2023 21:50

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core-util] Update "delay" method in core-util with abort options #23019

[core-util] Update "delay" method in core-util with abort options #23019

minhanh-phan commented Aug 26, 2022 •

edited

Loading

ghost commented Aug 26, 2022 •

edited by ghost

Loading

azure-sdk commented Aug 30, 2022 •

edited

Loading

HarshaNalluru Aug 31, 2022

minhanh-phan Sep 1, 2022 •

edited

Loading

joheredi Sep 1, 2022

HarshaNalluru Aug 31, 2022

minhanh-phan Sep 1, 2022

joheredi Sep 1, 2022

HarshaNalluru Sep 1, 2022

minhanh-phan Sep 1, 2022

jeremymeng Sep 1, 2022

HarshaNalluru left a comment

check-enforcer bot commented Sep 2, 2022

HarshaNalluru Sep 7, 2022

HarshaNalluru left a comment

xirzec left a comment

ghost commented Sep 8, 2022

[core-util] Update "delay" method in core-util with abort options #23019

[core-util] Update "delay" method in core-util with abort options #23019

Conversation

minhanh-phan commented Aug 26, 2022 • edited Loading

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

ghost commented Aug 26, 2022 • edited by ghost Loading

azure-sdk commented Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

minhanh-phan Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru left a comment

Choose a reason for hiding this comment

check-enforcer bot commented Sep 2, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Choose a reason for hiding this comment

HarshaNalluru left a comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

ghost commented Sep 8, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

minhanh-phan commented Aug 26, 2022 •

edited

Loading

Command used to generate this PR:***(Applicable only to SDK release request PRs)*

ghost commented Aug 26, 2022 •

edited by ghost

Loading

azure-sdk commented Aug 30, 2022 •

edited

Loading

minhanh-phan Sep 1, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.