chore(dtdl-parser): cleaning up and fixing bugs by YoDaMa · Pull Request #20300 · Azure/azure-sdk-for-js

YoDaMa · 2022-02-09T23:10:43Z

Some incremental changes:

Fix integration with iot-modelsrepository parser
Update readme

azure-sdk · 2022-02-09T23:19:02Z

API changes have been detected in @azure/dtdl-parser. You can review API changes here

API changes

- export declare type DtmiResolver = (identifiers: string[]) => Promise<string[] | null>;
+ export declare type DtmiResolver = (dtmis: string, options?: GetModelsOptions) => Promise<{
+     [dtmi: string]: unknown;
+ } | null>;

check-enforcer · 2022-02-09T23:36: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

olivakar · 2022-02-10T17:19:40Z

This seems like a change that needs to be taken to the parser node repo.

yes but we will integrate it into the monorepo to make the changes more permanent and easier to manage.

xirzec

A couple remarks

xirzec · 2022-02-10T20:29:25Z

Top level await requires Node 14+, our min bar is presently Node 12

will remove top level await

xirzec · 2022-02-10T20:30:47Z

@azure/iot-modelsrepository is added as a dev dependency, but this looks like a runtime dependency?

yeah this is something definitely need to sort out before GA. For now I'll move to dependencies to get the package working.

xirzec · 2022-02-10T20:32:52Z

mixing await and promise chaining is weird

Suggested change

const additionalJsonTexts = await this.dtmiResolver(undefinedIdentifiers).then((value: {[dtmi: string]: unknown}) => {

return Object.values(value).map((value) => JSON.stringify(value))

});

const value:{[dtmi: string]: unknown} = await this.dtmiResolver(undefinedIdentifiers);

const additionalJsonTexts = Object.values(value).map((value) => JSON.stringify(value));

maorleger · 2022-02-16T16:15:48Z

Do we have a general preference between { [dtmi: string]: unknown } and Record<string, unknown>? In this particular case IMO the former syntax is a little harder to parse with the promise and union.

fair. for now going to add this to the issue created to handle more feedback on the Parser before GA, to get this sorted out fully across the library.

I commented on the feedback issue, agreed we can sort this out more later since I don't think it would be considered breaking

YoDaMa · 2022-02-17T20:03:59Z

There are unresolved q's / comments in this PR that are out of scope of the work here. They've been moved to the issue tracking Preview Feedback. Other than that I hope to merge this and release the update today.

azure-sdk · 2022-02-17T20:04:42Z

API changes have been detected in @azure/dtdl-parser. You can review API changes here

API changes

- export declare type DtmiResolver = (identifiers: string[]) => Promise<string[] | null>;
-     dtmiResolver?: DtmiResolver;
+     getModels?: ModelsRepositoryClient["getModels"];

YoDaMa · 2022-02-17T20:20:26Z

@xirzec I'll need an approval from someone. @olivakar doesn't look to have the right permissions yet.

YoDaMa · 2022-02-17T22:15:42Z

I'm noticing errors on the build pipeline for browser tests in digital-twins-core.

TOTAL: 70 FAILED, 58 SUCCESS
Returned error code: 1
"@azure/digital-twins-core" failed to build.



==[ SUCCESS: 2 projects ]======================================================

These projects completed successfully:
  @azure/arm-digitaltwins    0.04 seconds
  @azure/dtdl-parser         15.08 seconds

==[ FAILURE: 1 project ]=======================================================

--[ FAILURE: @azure/digital-twins-core ]--------------------[ 34.32 seconds ]--

Invoking: karma start --single-run 
Projects failed to build.


17 02 2022 22:05:41.141:INFO [preprocessor.env]: Publishing variables:  [
  'AZURE_DIGITALTWINS_URL',
  'AZURE_CLIENT_ID',
  'AZURE_CLIENT_SECRET',
  'AZURE_TENANT_ID',
  'TEST_MODE'
]

  ...2520 lines omitted...
        at async tryGetAccessToken (../../core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts:85:26 <- dist-test/index.browser.js:32880:33)
        at async beginRefresh$2 (../../core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts:96:11 <- dist-test/index.browser.js:32888:18)
        at async BearerTokenAuthenticationPolicy.sendRequest (../../core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts:251:23 <- dist-test/index.browser.js:33009:32)
        at async ThrottlingRetryPolicy$1.sendRequest (../../core/core-http/src/policies/throttlingRetryPolicy.ts:69:16 <- dist-test/index.browser.js:33433:27)
        at async AzureDigitalTwinsAPI.sendOperationRequest (../../core/core-http/src/serviceClient.ts:522:23 <- dist-test/index.browser.js:35938:32)
        at async createModel (test/public/testRelationships.spec.ts:122:3 <- dist-test/index.browser.js:84750:10)
        at async setUpModels (test/public/testRelationships.spec.ts:127:3 <- dist-test/index.browser.js:84754:10)
        at async Context.<anonymous> (test/public/testRelationships.spec.ts:1289:5 <- dist-test/index.browser.js:85482:10)

TOTAL: 70 FAILED, 58 SUCCESS

I haven't made any changes to that folder I think. @xirzec am I reading the test failure correctly?

xirzec · 2022-02-17T22:23:00Z

I haven't made any changes to that folder I think. @xirzec am I reading the test failure correctly?

Hm looks like it's caused by the recorder not handling requests to identity during playback:

AuthenticationError: invalid_request Status code: 400
    More details:
    AADSTS90002: Tenant '12345678-1234-1234-1234-123456789012' not found. Check to make sure you have the correct tenant ID and are signing into the correct cloud. Check with your subscription administrator, this may happen if there are no active subscriptions for the tenant.
    Trace ID: 442a862c-6c4a-415d-9956-ab246c359b00

My first solution to confusing failures like this is to re-merge main and try again.

YoDaMa · 2022-02-17T22:33:15Z

My first solution to confusing failures like this is to re-merge main and try again.

yep I've force pushed a freshly rebased branch with two commits now for the changes. Let's see if that sorts it out...

YoDaMa requested review from bterlson, jeremymeng, johngallardo, ramya-rao-a and xirzec as code owners February 9, 2022 23:10

ghost added the Digital Twins label Feb 9, 2022

olivakar approved these changes Feb 10, 2022

View reviewed changes

xirzec reviewed Feb 10, 2022

View reviewed changes

maorleger reviewed Feb 16, 2022

View reviewed changes

YoDaMa requested review from deyaaeldeen, mikeharder and praveenkuttappan as code owners February 16, 2022 19:51

YoDaMa force-pushed the dtdl-parser/fix-readme branch from 3c3c421 to ba5bb6d Compare February 16, 2022 20:07

YoDaMa requested a review from witemple-msft as a code owner February 16, 2022 20:07

xirzec approved these changes Feb 17, 2022

View reviewed changes

YoDaMa added 2 commits February 17, 2022 16:09

dtdl-parser updates

b152a79

add spelling fixes

c4df98b

YoDaMa force-pushed the dtdl-parser/fix-readme branch from 3090881 to c4df98b Compare February 17, 2022 22:32

YoDaMa added 2 commits February 21, 2022 13:05

update release date

56a445c

Merge branch 'main' into dtdl-parser/fix-readme

ad795a2

HarshaNalluru mentioned this pull request Feb 22, 2022

[Digital Twins] Use "@azure-tools/test-credential"; #20494

Merged

YoDaMa merged commit 9124db3 into Azure:main Feb 22, 2022

-      const additionalJsonTexts = await this.dtmiResolver(undefinedIdentifiers).then((value: {[dtmi: string]: unknown}) => {
-        return Object.values(value).map((value) => JSON.stringify(value))
-      });
+      const value:{[dtmi: string]: unknown} = await this.dtmiResolver(undefinedIdentifiers);
+      const additionalJsonTexts  =  Object.values(value).map((value) => JSON.stringify(value));

Conversation

YoDaMa commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-sdk commented Feb 9, 2022

Uh oh!

check-enforcer Bot commented Feb 9, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xirzec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YoDaMa commented Feb 17, 2022

Uh oh!

azure-sdk commented Feb 17, 2022

Uh oh!

YoDaMa commented Feb 17, 2022

Uh oh!

YoDaMa commented Feb 17, 2022

Uh oh!

xirzec commented Feb 17, 2022

Uh oh!

YoDaMa commented Feb 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

YoDaMa commented Feb 9, 2022 •

edited

Loading