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.

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

[Scheduled-Query] update api version #3399

Merged
merged 9 commits into from
May 31, 2021
Merged

[Scheduled-Query] update api version #3399

merged 9 commits into from
May 31, 2021

Conversation

kairu-ms
Copy link
Contributor

@kairu-ms kairu-ms commented May 19, 2021 •
edited
Loading

closed Azure/azure-cli#17891

  • Add --skip-query-validation parameter
  • Add --check-ws-alerts-storage parameter
  • [Breaking Change] the default value of --mute-actions-duration is removed because alert auto mitigate is supported

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

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@kairu-ms kairu-ms requested a review from jsntcy as a code owner May 19, 2021 08:18
@yonzhan
Copy link
Collaborator

yonzhan commented May 19, 2021

Scheduled-Query

@yonzhan yonzhan requested a review from 00Kai0 May 19, 2021 09:20
@yonzhan yonzhan added this to the S187 milestone May 19, 2021
@yanivlavi
Copy link
Contributor

@kairu-ms
Flag '--mute-actions-duration' is not removed.
It is only supported when stateful is disabled. Stateful should be a flag control setting with default being enabled.

Please pass this PR with us for sign-off before you merge.

@yanivlavi
Copy link
Contributor

Please make sure you do not diverge from metric alerts as they also have 'auto-mitigate' option.
Keep in mind we want to be aligned with them.

@kairu-ms kairu-ms requested a review from yanivlavi May 20, 2021 03:11
@kairu-ms
Copy link
Contributor Author

kairu-ms commented May 20, 2021 •
edited
Loading

@kairu-ms
Flag '--mute-actions-duration' is not removed.
It is only supported when stateful is disabled. Stateful should be a flag control setting with default being enabled.

Please pass this PR with us for sign-off before you merge.

Thanks for your feedback, I have two question:

  1. What do you mean by stateful? Can user enable or disable stateful by cli?
  2. I don't remove the --mute-actions-duration flag. I've changed the default value of it to None. Do you want to remove it?

@yanivlavi
Copy link
Contributor

Thanks for your feedback, I have two question:

  1. What do you mean by stateful? Can user enable or disable stateful by cli?

Yes, it is controlled by the 'autoMitigate' flag in the API. Would like to have that controllable in the CLI.

  1. I don't remove the --mute-actions-duration flag. I've changed the default value of it to None. Do you want to remove it?

No, when 'autoMitigate' is false you can still set mute-actions-duration.
The defaults should be 'autoMitigate' true, mute-actions-duration is fine to be None as default.

@yanivlavi
Copy link
Contributor

What was the default for muting before?

@kairu-ms
Copy link
Contributor Author

What was the default for muting before?

The default value is 30 minutes

@kairu-ms
Copy link
Contributor Author

Thanks for your feedback, I have two question:

  1. What do you mean by stateful? Can user enable or disable stateful by cli?

Yes, it is controlled by the 'autoMitigate' flag in the API. Would like to have that controllable in the CLI.

  1. I don't remove the --mute-actions-duration flag. I've changed the default value of it to None. Do you want to remove it?

No, when 'autoMitigate' is false you can still set mute-actions-duration.
The defaults should be 'autoMitigate' true, mute-actions-duration is fine to be None as default.

I see. I will add that control in CLI.

@yonzhan yonzhan modified the milestones: S187, S188 May 21, 2021
jsntcy
jsntcy reviewed May 25, 2021
View reviewed changes
src/scheduled-query/azext_scheduled_query/_params.py
@@ -36,3 +37,6 @@ def load_arguments(self, _):
options_list=['--mute-actions-duration', '--mad'],
help='Mute actions for the chosen period of time (in ISO 8601 duration format) after the alert is fired.')
c.argument('actions', options_list=['--action', '-a'], action=ScheduleQueryAddAction, nargs='+', validator=get_action_group_validator('actions'))
c.argument('auto_mitigate', arg_type=get_three_state_flag(), help='The flag that indicates whether the alert should be automatically resolved or not. The default is true.')
c.argument('skip_query_validation', arg_type=get_three_state_flag(), help='The flag which indicates whether the provided query should be validated or not.')
c.argument('check_workspace_alerts_storage', options_list=['--check-ws-alerts-storage', '--cwas'], arg_type=get_three_state_flag(), help="The flag which indicates whether this scheduled query rule should be stored in the customer's storage.")
Copy link
Member

Choose a reason for hiding this comment

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

Add default value in help message

jsntcy
jsntcy approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@jsntcy jsntcy left a comment

Choose a reason for hiding this comment

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

:shipit:

@kairu-ms
Copy link
Contributor Author

Hi, @yanivlavi does this PR ready to release?

@kairu-ms kairu-ms force-pushed the scheduled-query-update-api-version branch from 587deac to 6e034b7 Compare May 31, 2021 03:27
@kairu-ms kairu-ms force-pushed the scheduled-query-update-api-version branch from 6e034b7 to eb2ac0d Compare May 31, 2021 07:26
@kairu-ms kairu-ms merged commit 5ed250a into Azure:master May 31, 2021
kairu-ms added a commit to kairu-ms/azure-cli-extensions that referenced this pull request Jun 21, 2021
@kairu-ms
Revert "[Scheduled-Query] update api version (Azure#3399)"
75217ef 
This reverts commit 5ed250a.
kairu-ms added a commit that referenced this pull request Jun 22, 2021
@kairu-ms
Revert "[Scheduled-Query] update api version (#3399)" (#3526)
b82ed96 
* Revert "[Scheduled-Query] update api version (#3399)"

This reverts commit 5ed250a.

* update extension version

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

@jsntcy jsntcy jsntcy approved these changes

@00Kai0 00Kai0 Awaiting requested review from 00Kai0

@yanivlavi yanivlavi Awaiting requested review from yanivlavi

Assignees

@kairu-ms kairu-ms

Labels
None yet
Projects
None yet
Milestone
S188
Development

Successfully merging this pull request may close these issues.

Scheduled query rules CMK check flag, stateful alerts and 1-minute frequency
4 participants
@kairu-ms @yonzhan @yanivlavi @jsntcy