Skip to content

feature/repairswagger#5000

Merged
dsgouda merged 2 commits intoAzure:psSdkJson6from
sumitabarahmand:feature/repairswagger
Nov 14, 2018
Merged

feature/repairswagger#5000
dsgouda merged 2 commits intoAzure:psSdkJson6from
sumitabarahmand:feature/repairswagger

Conversation

@sumitabarahmand
Copy link
Contributor

Description

SDK changes for the new Alert Repair admin operation
PR for the spec: Azure/azure-rest-api-specs#4084

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

@sumitabarahmand
Copy link
Contributor Author

@bganapa and @deathly809
Spec PR link:
Azure/azure-rest-api-specs#4084

@deathly809
Copy link
Member

@sumitabarahmand After your spec PR has been merged please regenerate from merged spec.

Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

When ready, please squash commits into a single commit

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update PackageReleaseNotes

Copy link
Member

Choose a reason for hiding this comment

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

please mention that repair api is supported only on AzureStack 1811 +

Copy link
Member

Choose a reason for hiding this comment

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

@dsgouda can we add a checklist item in the PR saying 'I have updated relevant package release notes' ?

Copy link
Member

Choose a reason for hiding this comment

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

I see that you already mention about 1811 :)

@bganapa
Copy link
Member

bganapa commented Nov 5, 2018

@dsgouda GitHub has the option of squashing the commits when you are merging the PR. We are ok for you to do this. Pls lets us know if you still want us to manually squash the commits

@shahabhijeet
Copy link
Contributor

@bganapa we lately started seeing issue with merges when we use squash/merge options, especially while trying to merge into other branches.
So we have now started being very particular on not having really long commit history. I understand 2-3 commits. But when you go beyond that, we ask RPs to clean up the commit history.
That is just something that helps us as well as you to have clean commit history, especially when we need to revert changes or when we have to create a branch out of a commit.
So help us to maintain the commit history in a clean state.

@bganapa
Copy link
Member

bganapa commented Nov 8, 2018

Yes, we will squash the commits and update this PR once the specs PR is merged Azure/azure-rest-api-specs#4084

@deathly809
Copy link
Member

@sumitabarahmand Your REST spec has been merged, please regenerate and make requested changes above. Thanks!

@dsgouda
Copy link
Contributor

dsgouda commented Nov 12, 2018

@azuresdkci retest this please

@bganapa
Copy link
Member

bganapa commented Nov 13, 2018

@sumitabarahmand could you please squash the commits..
Can you please add a release note saying the repair api is supported azurestack 1811 +

fix build issues

Add SDK unit test for Alert Repair

rabase and resolve conflicts

revert generate changes

update SDK based on updated alert spec

remove auto generated metadata

update assembly version

generate SDK with latest spec

add autogenerated spec metadata

generate SDK from merged specs

update release notes
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Few minor comments

Preview release of the InfrastructureInsights administrator SDK which allows administrators
1) View the health of their AzureStack stamp resources
2) View and manage alerts
Repair API is supported in azurestack 1811+
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please stick to numbering scheme

Copy link
Member

Choose a reason for hiding this comment

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

@dsgouda Should we only put in new things for release notes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally yes, that way users know what changed from the last version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just updated the PR.

/// <param name="closedByUserAlias">User alias who closed the
/// alert.</param>
public Alert(string id = default(string), string name = default(string), string type = default(string), IDictionary<string, string> tags = default(IDictionary<string, string>), string location = default(string), string closedTimestamp = default(string), string createdTimestamp = default(string), IList<IDictionary<string, string>> description = default(IList<IDictionary<string, string>>), string faultId = default(string), string alertId = default(string), string faultTypeId = default(string), string lastUpdatedTimestamp = default(string), IDictionary<string, string> alertProperties = default(IDictionary<string, string>), IList<IDictionary<string, string>> remediation = default(IList<IDictionary<string, string>>), string resourceRegistrationId = default(string), string resourceProviderRegistrationId = default(string), string severity = default(string), string state = default(string), string title = default(string), string impactedResourceId = default(string), string impactedResourceDisplayName = default(string), string closedByUserAlias = default(string))
/// <param name="hasValidRemediationAction">Indicates if the alert can
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this autogenerated or hand edited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Autogenerated

Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

LGTM

@dsgouda
Copy link
Contributor

dsgouda commented Nov 14, 2018

Will merge on CIs passing

@dsgouda dsgouda merged commit 73026ac into Azure:psSdkJson6 Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants