This repository has been archived by the owner on Dec 31, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
docs(samples): Adding template samples #136
docs(samples): Adding template samples #136
Changes from 5 commits
4e46538
0486123
35be7ec
d2b43a1
d027304
ee4d8ae
41512b9
758069e
83c1a1b
3896df2
a41ee95
deffb61
7301bbd
fe3d984
5d67138
80bb1d8
889b4aa
c6993cf
702ea0e
1e809cc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
generally the preferred style for newly written samples is one sample per file, as it allows for imports to be included in the region tags: https://github.com/GoogleCloudPlatform/python-docs-samples/blob/master/AUTHORING_GUIDE.md#region-tags
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.
Well, the guide you linked doesn't really state that there should be 1:1 ratio between files and samples and I've been doing multiple samples per file for some time now in this repo. See for example vm creation samples. This way imports are included in the samples just fine.
I've read the internal "Code Snippets 201" guide and figured that this is the best way to group the samples. I plan to reorganize the samples soon, to introduce more structure with folders for example. If there are reasons to introduce 1 sample = 1 file division, I'd like to know it.
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.
The reason is essentially enforcement/ease of use. With your samples, their is only one import so it's very complicated. However if a new import would be added for whatever reason, it should only apply to the sample using it, which becomes more complicated. Same with when an import needs to be removed - if it's no longer using it in the sample (but is in a different sample in the same file), flake8 can't catch that it's only being used in some region tags but not all.
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.
OK, I see your point. I will reorganize those samples soon, as we were planning to do a bigger refactoring of how we keep the samples for the Compute documentation in multiple languages.
Could you just suggest what would be the best course of action, when there are rather big pieces of code repeating in multiple samples like in the vm creation samples? There, the
create_with_disks
method is used by 6 different samples. Can a region snap multiple files? If not, I will have to replicate this code multiple times...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.
It depends on how you arrange the samples on the page. If there really is a certain amount of boiler plate, you could create a different region tag for that section and have a different region tag that references. However, it's probably feasible to just integrate it as part of the main samples.
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.
Should this say "...object that represents the..." instead? It's a little unclear why it would only describe the template rather than be the template itself.
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.
You're right, represents is better.
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 external IP:
"The template lets the instance use an external IP address through Cloud NAT."
I attempted to clarify, but correct me if I'm misunderstanding the behavior here.
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.
That's not that simple and I'm not sure how to describe it well... ONE_TO_ONE_NAT actually means the Cloud NAT is not involved (that's a weird, I know). But regular NAT routing takes place to deliver the traffic.
See here:
Cloud NAT is used when you have a bunch of VMs with only internal IPs and you want to give them Internet access. They can "share" one external IP then, to talk with the Internet and our Cloud NAT is making sure that the traffic is routed properly to those instances.
I'm honestly not sure about the origin on the ONE_TO_ONE_NAT value, why is it called like that... Perhaps it's because there's basically no address translation? The IP stays as is, so it's one-to-one like that?
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.
Same note as above. Does the InstanceTemplate object describe or represent the new template?
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.
Kind of both... But I like the represent notion, as it reminds that the actual template is somewhere out there in the cloud.
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.
It's unclear why this must match when other aspects can be changed. For example, you replace the image with a Rocky Linux image.
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.
Yes, but this tells the system which disk we're modifying.
So let's say this one instance has two disks: disk-1 and disk-2. One is a boot disk, other is just some data disk. The template we want to create is supposed to replace image for only one of those disks (i.e. disk-1). That's why it must match. Can you help with phrasing it clearly?
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.
As written, this raises some questions like "What else can I override?". If this change is specific to this example, we can probably change to just say "Override the auto_delete setting."
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.
You can't really override anything else. See here to see what the
DiskInstantiationConfig
can fit.I'll change to comment as suggested.