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

[MetricsAdvisor] Design: how Update methods should be designed #16172

Closed
kinelski opened this issue Oct 22, 2020 · 1 comment
Closed

[MetricsAdvisor] Design: how Update methods should be designed #16172

kinelski opened this issue Oct 22, 2020 · 1 comment
Labels
blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor
Milestone

Comments

@kinelski
Copy link
Member

Current approach has multiple flaws.

  • It takes an ID both as a method parameter and as a model property. For instance, UpdateHook takes a hookId parameter and a NotificationHook instance, which contains an Id property. Should we make Id settable and remove the string parameter?
  • .NET cannot differentiate between null and undefined. This is likely the main cause of [MetricsAdvisor] Fix Update sample issues #16168. One of the following scenarios will happen unintentionally:
    • User wants to update a property to null (clear it). However, our client doesn't send anything.
    • User doesn't want to change property. However, our client will send a null and clear it.
  • We use the same model for Create and Update methods. However, some properties are immutable after creation, so they are not used during update. Also, Update does not have required properties, so we shouldn't enforce them in the constructor (but we need to do so for Create).
@kinelski kinelski added Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor labels Oct 22, 2020
@kinelski kinelski added this to the [2020] November milestone Oct 22, 2020
@kinelski kinelski self-assigned this Oct 22, 2020
@kinelski kinelski modified the milestones: [2020] November, Backlog Oct 22, 2020
@kinelski kinelski modified the milestones: Backlog, [2021] February Jan 12, 2021
@kinelski kinelski added the blocking-release Blocks release label Jan 13, 2021
@kinelski kinelski removed their assignment Feb 5, 2021
@kinelski kinelski removed the blocking-release Blocks release label Feb 5, 2021
@kinelski kinelski modified the milestones: [2021] February, Backlog Feb 5, 2021
@kinelski kinelski modified the milestones: Backlog, [2021] May Apr 19, 2021
@kinelski kinelski added the blocking-release Blocks release label Apr 19, 2021
@kinelski kinelski modified the milestones: [2021] May, [2021] June May 7, 2021
@kinelski
Copy link
Member Author

kinelski commented Jun 4, 2021

We'll only support the scenario of calling Get followed by Update. Update methods will take the whole object, with no ID parameter. Changes already implemented and merged to master.

@kinelski kinelski closed this as completed Jun 4, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor
Projects
None yet
Development

No branches or pull requests

1 participant