-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: consume reservation samples #4407
base: main
Are you sure you want to change the base?
Conversation
Here is the summary of changes. You are about to add 2 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.
There are a couple core issues with this PR:
- There is more than one function per region tag
- It looks like there is a lot of duplication with existing samples.
) | ||
|
||
// Creates the reservation from given template in particular zone | ||
func createConsumableReservation(w io.Writer, projectID, zone, reservationName, sourceTemplate string) error { |
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.
issue: it looks like you're duplicating the same code, createConsumableReservation()
twice. Let's not do that. If the snippet needs this to run, then put all of this code into the test file.
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've done it in this way, to be as close as possible to the gcloud example in documentation.
There it is done in 2 steps:
- shows how to create a reservation (yeah, in this case, consuming any reservation, it just duplicates existing sample)
- actual consuming
Therefore, I thought it would be consistent to keep 2 functions. Do you think it's better to remove first and keep only consumeAnyReservation()?
) | ||
|
||
// Creates the reservation from given template in particular zone | ||
func createSpecificConsumableReservation(w io.Writer, projectID, zone, reservationName, sourceTemplate string) error { |
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.
issue: same as previous. If this function is required as a snippet, it should be in its own code file.
See: https://googlecloudplatform.github.io/samples-style-guide/#one-per-file
Also, it looks very similar to createConsumableReservation()
. Is there any code that makes it unique compared to the other function.
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.
Thanks for pointing out, but here is the same situation. In gcloud example it is separated into 2 steps, but this is still a single sample.
Is there any code that makes it unique compared to the other function.
Yes, there is additional field, which makes the difference to consuming
SpecificReservationRequired: proto.Bool(true)
I can make all actions being made in a single function, will it be better?
Description
Samples, which demonstrates how to create reservation, which can be consumed by:
doc link: https://cloud.google.com/compute/docs/instances/reservations-consume
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
go test -v ./..
(see Testing)gofmt
(see Formatting)go vet
(see Formatting)