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

feat: New Consistency group Samples : Add Disk, List Disks, Remove Disk #12796

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Thoughtseize1
Copy link
Contributor

@Thoughtseize1 Thoughtseize1 commented Nov 26, 2024

Description

New Consistency Group Samples for doc.page :

Region tag Filename
compute_consistency_group_add_disk add_disk_consistency_group.py
compute_consistency_group_disks_list list_disks_consistency_group.py
compute_consistency_group_remove_disk remove_disk_consistency_group.py

Checklist

@product-auto-label product-auto-label bot added samples Issues that are directly related to samples. api: compute Issues related to the Compute Engine API. labels Nov 26, 2024
@Thoughtseize1 Thoughtseize1 added the snippet-bot:force-run Force snippet-bot runs its logic label Nov 26, 2024
@snippet-bot snippet-bot bot removed the snippet-bot:force-run Force snippet-bot runs its logic label Nov 26, 2024
@Thoughtseize1 Thoughtseize1 marked this pull request as ready for review November 26, 2024 14:02
@Thoughtseize1 Thoughtseize1 requested review from a team as code owners November 26, 2024 14:02
@Thoughtseize1 Thoughtseize1 added the snippet-bot:force-run Force snippet-bot runs its logic label Nov 27, 2024
@snippet-bot snippet-bot bot removed the snippet-bot:force-run Force snippet-bot runs its logic label Nov 27, 2024
@Thoughtseize1 Thoughtseize1 changed the title feat: New Consistency group Samples : Add Disk, List Disks feat: New Consistency group Samples : Add Disk, List Disks, Remove Disk Nov 27, 2024
project_id: str,
disk_name: str,
disk_location: str,
disk_region_flag: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if replacing the disk_region_flag with disk_region_flag = disk_location[-1].isdigit() would make the sample more readable or easier to use? I personally always like when the code I want to use is able to automatically detect what kind of values I pass.

On the other hand, that is less explicit and introduces a very slight chance that the code will fail one day, if Google decides to change the region and zone naming schema.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your thoughts. I'm quite certain that if Google ever decides to change the naming policy for regions and zones, this code will break. In fact, I imagine many things would need fixing if such a change happens! 😅😬 But will it happen anytime soon? I don't think so :)

I also appreciate when code takes care of things for me automatically. Requiring users to explicitly set a disk_region_flag could lead to more errors on their part.
So, I'll make adjustments to the sample by modifying the disk creation and removing logic to make these functions more user-friendly 🙌

@Thoughtseize1 Thoughtseize1 added the snippet-bot:force-run Force snippet-bot runs its logic label Nov 28, 2024
@snippet-bot snippet-bot bot removed the snippet-bot:force-run Force snippet-bot runs its logic label Nov 28, 2024
@Thoughtseize1 Thoughtseize1 added the snippet-bot:force-run Force snippet-bot runs its logic label Nov 28, 2024
@snippet-bot snippet-bot bot removed the snippet-bot:force-run Force snippet-bot runs its logic label Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: compute Issues related to the Compute Engine API. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants