- Notifications
You must be signed in to change notification settings - Fork2k
feat: compute_consume_specific_shared_reservation#3912
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
snippet-botbot commentedOct 29, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated bysnippet-bot.
|
a9b6e6b toae2e6aaCompare| before(async()=>{ | ||
| projectId=awaitdisksClient.getProjectId(); | ||
| after(async()=>{ | ||
| // Cleanup resources |
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.
In this PR I also moved cleanup methods to after() to prevent keeping not used resources in our project after running the tests and to reduce the code.
ae2e6aa toee72ec9Compareee72ec9 tofd8bbd9Compare8a8a6d9 tod0f928aCompare672f6e6 to9c9e0d7Compare| * TODO(developer): Update these variables before running the sample. | ||
| */ | ||
| // The ID of the project where instance will be consumed and created. | ||
| constbaseProjectId='base-project-id'; |
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.
Can we rename to a more obvious names? LikereservationOwnerProjectID andreservationConsumerProjectID for example?
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.
@m-strzelczyk sure, fixed
9c9e0d7 toa5c5826CompareBigBlackWolf commentedDec 16, 2024
Hi@iennae, could you please take a look once again on this PR? cc:@rsamborski |
…ditional commentsObvious comments distract from the sample. Add clarifying comments that explain why and where users might customize.
iennae commentedDec 19, 2024
thanks@BigBlackWolf for the ping. Can you check the changes I made to the one file to clarify the comments? I think there is a lot of specific choices embedded in the samples that could use a little more detail and then some obvious comments that distract from the code. For example the instantiates aren't giving more context to the sample consumers. Could you make updates to the other files to match that style? In general, I know there are other samples like this, but it really feels import with the number of choices that folks have with compute to better understand how to manage that complexity rather than hide it and end up with copied patterns over time with no understanding of the "why". Thank you! |
iennae left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Aside from the comments needing improvement, I have some additional concerns. Refactoring to eliminate duplicate code and create helper functions would enhance readability and maintainability. These new helper functions could accept parameters like disk size, source image, and network settings, simplifying updates to this collection of samples and streamlining the samples across different attributes. Currently, the handcrafted samples require extensive reviews and introduce duplicate code.
I will seek a second opinion from the team tomorrow and update this PR accordingly. While I lean towards a conditional approval, I believe we can enhance consistency and style.
Thank you for your patience.
BigBlackWolf commentedDec 19, 2024
I appreciate your review@iennae and thanks for adjusting comments, imho they became much more informative! |
iennae commentedDec 21, 2024
Heya, the problem is the other issues I raised and this PR has made me realize there is a lot of repetition in these set of samples whoch is going to make them harder to maintain. Do we have an assigned engineering team that will be ok maintaining these? |
BigBlackWolf commentedDec 23, 2024
Unfortunately, I am not aware of those. |
iennae commentedDec 23, 2024
I think refactoring the new sample that is getting added since it has the same duplicate functions could happen in this PR. Do you know if there is an engineering team that will take on maintenance? My primary concern is the folks who have to maintain these. To be clear, I'm nit saying all the existing samples need to get updated but the new one should be refactoring to make this duplication code maintainable. |
m-strzelczyk commentedJan 21, 2025
@iennae I agree that the Compute code samples suffer from a lot of code repetition, which makes the code samples harder to maintain and read. However, helper functions are not that simple to implement with the way our code sample system works. We have two ways to work with this:
After years of using the Python way of "compiling" code samples, I'm not sure it was the best way of doing things. I hate code repetition, but on the other hand, I want our users, when faced with a single code sample, to have everything available to them in a very approachable way. It wouldn't be good if we, for example, used a helper function in a code samples and later forgot to include it in the documentation next to the code sample. That said, I'll have a look at the code and comments to address your other suggestions. |
glasnt commentedMar 5, 2025
CLA check override note: previous executions workedhttps://github.com/GoogleCloudPlatform/nodejs-docs-samples/pull/3912/checks?check_run_id=34225819577 |
Uh oh!
There was an error while loading.Please reload this page.
Description
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test(seeTesting)npm run lint(seeStyle)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.