Skip to content

Rename Insights client to Monitor#894

Closed
lmazuel wants to merge 1 commit intoAzure:masterfrom
lmazuel:insights_monitor
Closed

Rename Insights client to Monitor#894
lmazuel wants to merge 1 commit intoAzure:masterfrom
lmazuel:insights_monitor

Conversation

@lmazuel
Copy link
Copy Markdown
Member

@lmazuel lmazuel commented Jan 31, 2017

Discussed with the Monitor team, but of course needs confirmation here :)

FYI @gucalder @amarzavery @olydis

Copy link
Copy Markdown
Contributor

@gucalder gucalder left a comment

Choose a reason for hiding this comment

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

Sorry, I did not find a way to write comments closer to the source.

The name change is Ok, but didn't we also have issues with the folder name? It is usually confused with app insights. Then we receive PRs to this folders, but their are intended to go to app insights.

If only changing the name of the package itself is enough, then I'll do the changes in the SDKs as soon as this is checked in.

It is a breaking change to the SDKs. I think we might need to warn the customers and wait several iterations, unless we can hide the change in the SDKs so that PowerShell, CLI, etc. do not see any change.

@vishrutshah
Copy link
Copy Markdown
Contributor

@lmazuel Changes looks good. I'd agree with you guys that changing the folder name "arm-insights" to "arm-monitor" and "insights" to "monitor" makes sense but I am fine if we do it now or afterwards as well.

@kirthik
Copy link
Copy Markdown
Contributor

kirthik commented Feb 1, 2017

@vishrutshah : Is this ready to be merged?

@vishrutshah
Copy link
Copy Markdown
Contributor

@kirthik I'd wait until we have approval from @gucalder.

@lmazuel What do you think? And what did you decide on renaming folder? I am fine with afterwards as well.

@lmazuel
Copy link
Copy Markdown
Member Author

lmazuel commented Feb 2, 2017

@vishrutshah I think the folders should be renamed at the same time, this way if a CI or a script is using this Swagger, it will break before generating a new client name (that has a risk to do not be seen). I discussed it in separate email thread with @gucalder . I agree only @gucalder is able to confirm the folder renaming and the merging of this PR. I will be happy to rename the folders in this PR directly if we have the "go" for it.

@vishrutshah
Copy link
Copy Markdown
Contributor

@gucalder Could you please provide us confirmation on the renames?

@kirthik
Copy link
Copy Markdown
Contributor

kirthik commented Feb 16, 2017

@gucalder : Could you please respond to @vishrutshah's question?

@gucalder
Copy link
Copy Markdown
Contributor

This PR is covered by #936.
So this can be closed without merge. I am going to keep Insights as it is until we remove it at some point in time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants