Skip to content

Conversation

@wanlwanl
Copy link
Contributor

Description

Add a new option --enable-messaging-logs for signalr, which can setup the EnableMessagingLogs in FeatureFlags. It's used to control whether the signalr service generate messaging logs or not.

Testing Guide

Use az signalr update -n <SIGNALR_NAME> -g <RESOURCE_GROUP> --enable-messaging-logs True to make signalr service start to generate messaing log for signalr instance SIGNALR_NAME in resource group RESOURCE_GROUP.

Use azdev test signalr to test including az signalr create and az signalr update

History Notes

[SIGNALR] az signalr create: Add new option --enable-messaging-logs for controling service generate messaging logs or not
[SIGNALR] az signalr update: Add new option --enable-messaging-logs for controling service generate messaging logs or not


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

@yonzhan yonzhan added this to the S177 milestone Sep 28, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Sep 28, 2020

SIGNALR

@yonzhan yonzhan requested a review from Juliehzl September 29, 2020 01:57

def signalr_create(client, signalr_name, resource_group_name,
sku, unit_count=1, location=None, tags=None, service_mode='Default', allowed_origins=None, default_action="Allow"):
sku, unit_count=1, location=None, tags=None, service_mode='Default', enable_message_logs='False', allowed_origins=None, default_action="Allow"):
Copy link
Member

@zackliu zackliu Sep 29, 2020

Choose a reason for hiding this comment

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

When using get_three_state_flag, the parameter should be a bool instead of string. Then, user can still use the lowercase true/false in commend line


def signalr_create(client, signalr_name, resource_group_name,
sku, unit_count=1, location=None, tags=None, service_mode='Default', allowed_origins=None, default_action="Allow"):
sku, unit_count=1, location=None, tags=None, service_mode='Default', enable_message_logs='False', allowed_origins=None, default_action="Allow"):
Copy link
Contributor

Choose a reason for hiding this comment

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

please make enable_messahe_logs=False, not 'False'

tags_key = 'key'
tags_val = 'value'
service_mode = 'Classic'
enable_message_logs = 'True'
Copy link
Contributor

Choose a reason for hiding this comment

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

Use True not 'True'


# Test update
self.cmd('az signalr update -n {signalr_name} -g {rg} --sku {updated_sku} --tags {updated_tags} --service-mode {update_service_mode}',
self.cmd('az signalr update -n {signalr_name} -g {rg} --sku {updated_sku} --tags {updated_tags} --service-mode {update_service_mode} --enable-message-logs {update_enable_message_logs}',
Copy link
Contributor

Choose a reason for hiding this comment

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

please add test for --enable-message-logs false in not string variable.
Check value with boolean False, not string if the output type is boolean for features[1].value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Juliehzl Test added for --enable-message-logs false.
The check value is string since the SignalrFeature.value is a string.

Copy link
Contributor

@Juliehzl Juliehzl left a comment

Choose a reason for hiding this comment

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

LGTM

@haroldrandom haroldrandom merged commit 7b194ef into Azure:dev Oct 16, 2020
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