Skip to content

Kusto 2020-06-14 PS version #12567

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Aug 11, 2020
Merged

Kusto 2020-06-14 PS version #12567

merged 13 commits into from
Aug 11, 2020

Conversation

@astauben
Copy link
Contributor

@astauben astauben commented Aug 4, 2020 •
edited by isra-fel
Loading

Description

Design review: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/685

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@adxsdkps
Copy link
Collaborator

adxsdkps commented Aug 4, 2020

Can one of the admins verify this patch?

@isra-fel isra-fel self-assigned this Aug 5, 2020
isra-fel
isra-fel requested changes Aug 7, 2020
View reviewed changes
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

Hi @astauben please see my inline comments.
Besides, it would be great if you could update the examples with the new parameters.

src/Kusto/.gitignore Outdated
/*.psm1
/*.snk
/*.csproj
/*.nuspec No newline at end of file
Copy link
Member

@isra-fel isra-fel Aug 7, 2020

Choose a reason for hiding this comment

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

Please ignore tools folder as well and don't check in any files in it

Copy link
Contributor Author

@astauben astauben Aug 9, 2020

Choose a reason for hiding this comment

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

done

Copy link
Member

@isra-fel isra-fel Aug 10, 2020

Choose a reason for hiding this comment

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

Some of the files are already checked in, e.g. src/Kusto/tools/Resources/examples/readme.md. Please remove the whole tools/ folder and commit again. Thanks

Copy link
Contributor Author

@astauben astauben Aug 10, 2020

Choose a reason for hiding this comment

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

done

src/Kusto/test/Add-AzKustoDatabasePrincipal.Tests.ps1 Outdated

Describe 'Add-AzKustoDatabasePrincipal' {
It 'AddExpanded' -skip {
{ throw [System.NotImplementedException] } | Should -Not -Throw
Copy link
Member

@isra-fel isra-fel Aug 7, 2020

Choose a reason for hiding this comment

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

Why are test cases all skipped?

Copy link
Contributor Author

@astauben astauben Aug 9, 2020

Choose a reason for hiding this comment

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

we are testing the scenario in the New-AzKustoDatabasePrincipalAssignment file.

Copy link
Member

@isra-fel isra-fel Aug 10, 2020

Choose a reason for hiding this comment

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

Can you just delete this file?

Copy link
Contributor Author

@astauben astauben Aug 10, 2020

Choose a reason for hiding this comment

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

done

@astauben
Copy link
Contributor Author

astauben commented Aug 9, 2020 •
edited
Loading

Hi @astauben please see my inline comments.
Besides, it would be great if you could update the examples with the new parameters.

Hi, @isra-fel Thanks for the review. all comments are resolved

@isra-fel isra-fel assigned astauben and unassigned isra-fel Aug 10, 2020
@astauben astauben requested a review from isra-fel August 10, 2020 12:17
isra-fel
isra-fel approved these changes Aug 11, 2020
View reviewed changes
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

LGTM

@isra-fel isra-fel merged commit 6972661 into Azure:generation Aug 11, 2020
@LucasYao93 LucasYao93 mentioned this pull request Aug 12, 2020
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@isra-fel isra-fel isra-fel approved these changes

Assignees

@astauben astauben

Labels

Cmdlet Review Complete Generator Related to cmdlet generator needs-revision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@astauben @adxsdkps @isra-fel @azuresdkci @roshauli