-
Notifications
You must be signed in to change notification settings - Fork 25
Conversation
665f08f
to
a363752
Compare
Here is the summary of changes. You are about to add 8 region tags.
This comment is generated by snippet-bot.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added couple suggestions.
import uuid | ||
rule_name = "firewall-sample-" + uuid.uuid4().hex[:10] | ||
print(f'Creating firewall rule {rule_name}...') | ||
create_firewall_rule(default_project_id, rule_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For usage in default values presentation we should probably mention that this will get priority of 1000. This code is not part of the sample, so maybe it should be added in the create_firewall_rule - similar to how we did it in set_usage_reports examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment, but I'm not sure how to gracefully add this to the create_firewall_rule sample, without adding the whole code block, which also contains call to functions from outside this sample.
I also expanded the comment in the create_firewall_rule
function, to better explain what's going on around the priority.
Co-authored-by: Remigiusz Samborski <rsamborski@users.noreply.github.com>
@dinagraves can you please review? I don't think this sample will change in any meaningful way. |
from sample_firewall import ( | ||
create_firewall_rule, | ||
patch_firewall_priority, | ||
delete_firewall_rule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetical order please!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - please fix the Lint issue! Imports are in incorrect order.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes b/198588179