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 add command #13 #860

Closed
wants to merge 2 commits into from
Closed

Adds graph schemaextension add command #13 #860

wants to merge 2 commits into from

Conversation

ypcode
Copy link
Contributor

@ypcode ypcode commented Mar 9, 2019

The owner argument is actually made mandatory, whenever trying calling the Graph without this argument returns an Unauthorized error

@ypcode ypcode changed the title Adds graph schemaextension add command #13 Adds graph schemaextension add command #13 Mar 9, 2019
@coveralls
Copy link

coveralls commented Mar 9, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling fe6c42b on ypcode:dev-schemaextension-add into 33c9780 on pnp:dev.

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Mar 9, 2019

Thanks @ypcode! I'll review it shortly

@waldekmastykarz waldekmastykarz self-assigned this Mar 9, 2019
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Hey @ypcode, nicely done! Would you mind doing a few changes before we merge it? Thanks!

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Mar 23, 2019

Thank you for the quick turnaround @ypcode 👏

Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Nicely done with good detailed docs and a few things I've fixed when merging the PR.

@@ -69,6 +74,22 @@ export default class Utils {
return value.toLowerCase() === 'true' || value.toLowerCase() === 'false'
}


public static isValidJsonString(value: string): IValidJsonCheckResult {
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 29, 2019

Choose a reason for hiding this comment

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

I don't think that we need something so complex for a check that can be easily done in the command

}

const targetTypes: string[] = args.options.targetTypes.split(',').map(t => t.trim());
let properties: any = JSON.parse(args.options.properties);
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 29, 2019

Choose a reason for hiding this comment

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

This should be const

};
}

private _validatePropertiesStringValue(properties: string): boolean {
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 29, 2019

Choose a reason for hiding this comment

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

Following the validation pattern, this method should return boolean | string. If the specified value is valid, it would return true, if not, then it would return the validation error message which would tell us exactly what the problem is (invalid JSON string vs. invalid property).

}

const invalidProps = (properties as any[]).filter(p => !p.name || !this._isValidPropertyType(p.type));
console.log('invalid props: ');
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 29, 2019

Choose a reason for hiding this comment

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

In the CLI we use cmd.log instead of console.log to properly integrate with the CLIs infrastructure

return false;
}

switch (propertyType.toLowerCase()) {
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 29, 2019

Choose a reason for hiding this comment

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

This would be helpful, if specifying property type was case-insensitive. However, for example with Integer, if you specify anything else than Integer, the request is rejected by Graph. So as such, we should check the value for proper casing accepted by the Graph.

## Usage

```sh
graph schemaextension add <options>
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 29, 2019

Choose a reason for hiding this comment

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

Should be [options] following the help displayed in the command


To create a schema extension, you have to specify a unique ID for the schema extension
You can assign a value in one of two ways:
- Concatenate the name of one of your verified domains with a name for the schema extension to form a unique string in this format, {domainName}_{schemaName}. As an example, contoso_mySchema. NOTE: Only verified domains under the following top-level domains are supported: .com,.net, .gov, .edu or .org.
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 29, 2019

Choose a reason for hiding this comment

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

Missing an empty line between the paragraph and the bullet list


When specifying the JSON string of properties on Windows, you
have to escape double quotes in a specific way. Considering the following
value for the _properties_ option: {"Foo":"Bar"},
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 29, 2019

Choose a reason for hiding this comment

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

We can't use markdown formatting in command help

## Examples

Create a schema extension
```sh
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 29, 2019

Choose a reason for hiding this comment

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

Missing empty line between a paragraph and a code block


Create a schema extension
```sh
graph schemaextension add --id MySchemaExtension --description "My Schema Extension" --targetTypes Group --owner 62375ab9-6b52-47ed-826b-58e47e0e304b --properties \`"[{""name"":""myProp1"",""type"":""Integer""},{""name"":""myProp2"",""type"":""String""}]\`
Copy link
Member

@waldekmastykarz waldekmastykarz Mar 29, 2019

Choose a reason for hiding this comment

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

No need to indent the code block

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Mar 31, 2019

Merged manually. Thank you! 👏

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.

None yet

3 participants