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

Adds graph schemaextension set command #1279

Closed
wants to merge 1 commit into from
Closed

Adds graph schemaextension set command #1279

wants to merge 1 commit into from

Conversation

ypcode
Copy link
Contributor

@ypcode ypcode commented Dec 19, 2019

PR for issue #15

@coveralls
Copy link

coveralls commented Dec 19, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 8c55a67 on ypcode:dev-schemaextension-update into 520f164 on pnp:dev.

@garrytrinder
Copy link
Member

garrytrinder commented Dec 19, 2019

Thanks @ypcode we will review shortly

garrytrinder pushed a commit to garrytrinder/cli-microsoft365 that referenced this pull request Jan 3, 2020
@waldekmastykarz waldekmastykarz self-assigned this Jan 12, 2020
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Hey @ypcode, would you mind doing a couple of changes before we merge it in? Thanks!

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jan 18, 2020

Hey @ypcode, have you noticed the drop in coverage?

@ypcode
Copy link
Contributor Author

ypcode commented Jan 19, 2020

Hi Waldek, sorry, I indeed did not, actually coverage reports many issues on my local dev, and not even all tests pass

======= Coverage summary ====
Statements   : 99.92% ( 63627/63676 )
Branches     : 99.06% ( 12456/12574 )
Functions    : 99.86% ( 5149/5156 )
Lines        : 99.92% ( 63627/63676 )
========================
1) spfx project externalize
       covers all text report branches:

      AssertionError [ERR_ASSERTION]: 124 == 122
      + expected - actual

      -124
      +122

      at Context.it (dist\o365\spfx\commands\project\project-externalize.spec.js:663:16)

But at various locations unrelated with my PR... it was even reporting this when I recently fetch all the latest from scratch (I was guessing it was normal...) I did not realize there was a drop on my side... sorry for that... should be good now, Coveralls says it's 100% :)

But then I don't get why it's not 100% on my side... is it related to my Windows environment ?

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jan 19, 2020

We recently switched to c8 for reporting coverage and it requires Node v12 to run properly. Could it be that you're running an older version of Node?

@ypcode
Copy link
Contributor Author

ypcode commented Jan 19, 2020

@ypcode
Copy link
Contributor Author

ypcode commented Jan 19, 2020

Okay coverage is now 100% on my local dev host as well with node 12.
Anyway I still have tests failing on spfx externalize project but I don't think it is anyhow related...

@ypcode ypcode requested a review from waldekmastykarz Jan 19, 2020
@waldekmastykarz
Copy link
Member

waldekmastykarz commented Jan 20, 2020

If you've updated your code, please push the code to your PR branch. Perhaps the issues you're seeing have been already fixed in the upstream.

Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

We're almost there! If you could do a few more fixes, we'd get it merged 👍

public get description(): string {
return 'Updates a Microsoft Graph schema extension';
}

Copy link
Member

@waldekmastykarz waldekmastykarz Jan 25, 2020

Choose a reason for hiding this comment

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

We miss tracking the telemetry for the different options.

Copy link
Contributor Author

@ypcode ypcode Jan 27, 2020

Choose a reason for hiding this comment

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

I added with only status and targetTypes which are the only standard parameters, all other are "user-defined" according to me, what do you think ?

Copy link
Member

@waldekmastykarz waldekmastykarz Jan 29, 2020

Choose a reason for hiding this comment

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

Looking at how options are defined in the command, description, status, targetTypes and properties are optional so we should track which of them are being used. Since the status is an enum, we could track the actual value.

@ypcode ypcode requested a review from waldekmastykarz Jan 27, 2020
@VelinGeorgiev VelinGeorgiev self-assigned this Jan 31, 2020
Copy link
Contributor

@VelinGeorgiev VelinGeorgiev left a comment

Nicely done @ypcode ! Thanks for taking time to do all the changes. This command is ready to be merged from here: https://github.com/VelinGeorgiev/office365-cli/tree/schema-set

You can see the minor changes I made in the comments.

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Feb 9, 2020

Merged manually. Thank you! 👏

@waldekmastykarz waldekmastykarz linked an issue Feb 9, 2020 that may be closed by this pull request
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.

Update Microsoft Graph schema extension
5 participants