fix(job): delete old region tag update_job_with_field_mask #9940

OremGLG · 2024-12-30T19:29:25Z

Description

Delete old region tag "update_job_with_field_mask"

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

ghost

Hi OremGLG and reviewers,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:

This pull request aims to clean up the jobs/v3 sample by removing an outdated region tag. Specifically, it removes the update_job_with_field_mask tag from the BasicJobSample.java file. This addresses issue b/347826098.

Here's a breakdown of the changes:

File: jobs/v3/src/main/java/com/google/samples/BasicJobSample.java
- Lines 133 and 150 were modified to remove the outdated // [START update_job_with_field_mask] and // [END update_job_with_field_mask] tags.

The changes are straightforward and effectively remove the obsolete tag, improving the clarity and maintainability of the sample code.

And now, a little haiku to lighten the mood:

Old tags must now fade,
Clean code shines, a brighter day,
Review, then merge on!

Thanks,
Gemini

snippet-bot · 2024-12-30T19:29:40Z

Here is the summary of changes.

You are about to delete 1 region tag.

jobs/v3/src/main/java/com/google/samples/BasicJobSample.java:133, tag update_job_with_field_mask

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

ghost

This pull request correctly removes the redundant region tag as described. A few minor points could improve the overall quality:

README Updates: Please confirm that the necessary README updates have been made to reflect the removal of the update_job_with_field_mask example. If no updates are needed, please explicitly state that in the PR description.
pom.xml Parent: Ensure the pom.xml parent is set to the latest shared-configuration version and mark the corresponding checklist item.
Pre-submit Checks: While CI is in place, running tests and linters locally before submitting can save time and resources by catching issues early.
Javadoc Comment: Adding a concise Javadoc comment to the updateJobWithFieldMask method would improve code clarity. This aligns with the Google Java Style Guide's emphasis on clear documentation.

ghost · 2024-12-30T19:29:55Z

jobs/v3/src/main/java/com/google/samples/BasicJobSample.java

@@ -130,7 +130,6 @@ public static Job updateJob(String jobName, Job jobToBeUpdated) throws IOExcepti
  // [END job_update_job]

  // [START job_update_job_with_field_mask]
-  // [START update_job_with_field_mask]



Since the redundant tag has been removed, it's a good opportunity to add a Javadoc comment explaining the purpose of the updateJobWithFieldMask method. This improves code clarity and maintainability, aligning with the Google Java Style Guide's recommendation for clear and concise comments.

Suggested change

/**

* Updates a job using a field mask to specify which fields to modify.

*/

ghost · 2024-12-30T19:29:55Z

jobs/v3/src/main/java/com/google/samples/BasicJobSample.java

@@ -147,7 +146,6 @@ public static Job updateJobWithFieldMask(String jobName, String fieldMask, Job j
      throw e;
    }
  }
-  // [END update_job_with_field_mask]
  // [END job_update_job_with_field_mask]



Now that the region tag update_job_with_field_mask has been removed from this file, it's important to verify there are no remaining references to it in other parts of the codebase or documentation. Please double-check to ensure consistency across the project.

ghost

Hi OremGLG and reviewers,

I'm currently reviewing this pull request and will post my full review in a few minutes. In the meantime, here's a quick summary to get everyone up to speed:

This pull request aims to fix issue b/347826098 by deleting an outdated region tag from the BasicJobSample.java file. Specifically, the changes involve removing the // [START update_job_with_field_mask] and // [END update_job_with_field_mask] tags from the updateJobWithFieldMask method.

Here's a breakdown of the changes:

File: jobs/v3/src/main/java/com/google/samples/BasicJobSample.java
- Lines 133-134 and 150-151: Removed the outdated region tags.

The intent is to clean up the code and remove unnecessary or obsolete comments. The changes are straightforward and should not introduce any functional changes.

A little haiku to brighten your day:

Old tags now removed,
Code cleaned, a brighter view,
Reviews flow smoothly.

Best,
Gemini

ghost

This pull request correctly removes the redundant region tag update_job_with_field_mask, improving the code's clarity. The PR description is well-written, the checklist is thorough, and the change is small and straightforward. A minor suggestion would be to add a small entry to the README reflecting this change, if applicable. I've referenced the Google Java Style Guide in this review.

One small improvement could be to consolidate the two region tags into a single one, as the start and end tags now appear consecutively. This adheres to the principle in the Google Java Style Guide of keeping comments concise and relevant.

ghost

Hi OremGLG and reviewers,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:

This pull request aims to clean up the jobs/v3 sample by removing an outdated region tag. Specifically, it removes the update_job_with_field_mask tag from the BasicJobSample.java file. This addresses issue b/347826098.

Here's a breakdown of the changes:

File: jobs/v3/src/main/java/com/google/samples/BasicJobSample.java
- Lines 133 and 150 were modified to remove the outdated region tag // [START update_job_with_field_mask] and // [END update_job_with_field_mask]. The code functionality remains unchanged.

The changes are straightforward and well-documented. The removal of the outdated tag improves the clarity and maintainability of the sample code.

And now, a little haiku to lighten the mood:

Old tag, now removed,
Code cleaner, bright and new,
Review is a breeze.

Thanks!
Gemini

ghost

This pull request correctly removes the redundant region tags associated with the removal of the update_job_with_field_mask sample. The provided context mentions adherence to the Google Java Style Guide, and this change aligns well with that guide by removing clutter and keeping the code concise. The PR title and description clearly explain the change. However, the checklist mentions updating the README. Please ensure the README is updated to reflect the sample removal. If no README update is required, please explicitly state that in the PR description or checklist.

jobs/v3/src/main/java/com/google/samples/BasicJobSample.java

ghost

Hi OremGLG and reviewers,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to get everyone up to speed:

This pull request aims to fix issue b/347826098 by deleting an outdated region tag from the BasicJobSample.java file. Specifically, the changes involve removing the // [START update_job_with_field_mask] and // [END update_job_with_field_mask] tags from the updateJobWithFieldMask method.

Here's a breakdown of the changes:

File: jobs/v3/src/main/java/com/google/samples/BasicJobSample.java
- Lines 133-134 and 150-151: Removed the outdated region tags.

The intent is to clean up the code and remove unnecessary or obsolete comments, improving code readability and maintainability. The changes are straightforward and well-documented.

A little haiku to brighten your day:

Old tags now removed,
Code shines, clean and bright anew,
Review's swift and smooth.

Thanks,
Gemini

ghost

This pull request correctly removes the redundant region tag update_job_with_field_mask, improving the code's clarity. The checklist is thorough, and marking this PR for merging after approval is appropriate. I've summarized the Google Java Style Guide elements I referenced below.

Google Java Style Guide Summary:

Javadoc: Ensured proper Javadoc formatting for comments.

It would be beneficial to add a unit test to verify the functionality of updateJobWithFieldMask after this change, even though the change itself is minor. This helps ensure that removing the tag doesn't inadvertently affect the method's behavior and provides better regression testing.

ghost · 2024-12-30T19:31:15Z

jobs/v3/src/main/java/com/google/samples/BasicJobSample.java

@@ -130,7 +130,6 @@ public static Job updateJob(String jobName, Job jobToBeUpdated) throws IOExcepti
  // [END job_update_job]

  // [START job_update_job_with_field_mask]
-  // [START update_job_with_field_mask]

  /** Update a job. */


Since this is a public method, it should have a Javadoc comment explaining its purpose, parameters, and return value, per the Google Java Style Guide. I'm recommending this change to adhere to the Google Java Style Guide. Adding clear documentation improves the maintainability and understandability of the code.

Suggested change

/** Update a job. */

/**

* Updates a job using a field mask.

*

* @param jobName The resource name of the job to update.

* @param fieldMask A comma-separated list of fields to update.

* See https://cloud.google.com/sdk/gcloud/reference/run/jobs/update#--update-mask for details.

* @param jobToBeUpdated The job object containing the updated fields.

* @return The updated job.

* @throws IOException if an I/O error occurs during the update operation.

*/

minherz

lgtm. confirm no use for the deleted region tag in documentation.
consider to look into bot's recommendations for minor improvements.

OremGLG · 2025-01-06T15:35:19Z

lgtm. confirm no use for the deleted region tag in documentation. consider to look into bot's recommendations for minor improvements.

Thanks for the suggestion. The tag is not used in documentation because it was replaced with the new one.
https://devrel.corp.google.com/snippets/update_job_with_field_mask

* chore(job): migrate regions by associating them with an official product with a job_ prefix (#9883) * chore(endpoints): delete region 'swagger' in endpoints/multiple-versions (#9857) * chore(endpoints): delete region swagger to openapi-v1.yaml * chore(endpoints): delete region swagger to openapi-v2.yaml * chore(job): delete sample jobs_java_dependencies_beta (#9810) * chore(job): delete sample jobs_java_dependencies_beta * chore(job): delete region_tab 'jobs_java_dependencies_beta' and update 'google-api-services-jobs' version * feat(compute): add compute disk regional replicated sample (#9697) * Implemented compute_disk_regional_replicated sample, created test * Fixed zone * Fixed test * Fixed test * Fixed disk size * Fixed code as requested in the comment * feat(compute): add compute disk start/stop replication samples (#9650) * Implemented compute_disk_start_replication and compute_disk_stop_replication samples, created tests * Fixed test * Deleted not related classes * Fixed lint issue * Increased timeout * Split samples for zonal location * Fixed code * Fixed code * Increased timeout * Increased timeout * feat(tpu): add tpu vm create spot sample. (#9610) * Changed package, added information to CODEOWNERS * Added information to CODEOWNERS * Added timeout * Fixed parameters for test * Fixed DeleteTpuVm and naming * Added comment, created Util class * Fixed naming * Fixed whitespace * Split PR into smaller, deleted redundant code * Implemented tpu_vm_create_spot sample, created test * changed zone * Changed zone * Fixed empty lines and tests, deleted cleanup method * Changed zone * Deleted redundant test class * Increased timeout * Fixed test * feat(tpu): add tpu vm create startup script sample. (#9612) * Changed package, added information to CODEOWNERS * Added information to CODEOWNERS * Added timeout * Fixed parameters for test * Fixed DeleteTpuVm and naming * Added comment, created Util class * Fixed naming * Fixed whitespace * Split PR into smaller, deleted redundant code * Implemented tpu_vm_create_startup_script sample, created test * Fixed tests and empty lines * Changed zone * Deleted redundant test classes * Increased timeout * Fixed code * feat(tpu): add tpu queued resources create/get/delete samples (#9613) * Changed package, added information to CODEOWNERS * Added information to CODEOWNERS * Added timeout * Fixed parameters for test * Fixed DeleteTpuVm and naming * Added comment, created Util class * Fixed naming * Fixed whitespace * Split PR into smaller, deleted redundant code * Implemented tpu_queued_resources_create, tpu_queued_resources_get, tpu_queued_resources_delete_force and tpu_queued_resources_delete samples, created tests * Fixed test * Fixed tests * Fixed error massage * Fixed typo * Fixed zone * Fixed test * Fixed code * Deleted commented imports * Fixed code as requested in comments * feat(tpu): add tpu queued resources create spot (#9615) Add a code sample for tpu_queued_resources_create_spot * chore: add translate dev team for translate samples (#9888) b/385243174 * feat(securitycenter): Add Resource SCC Management API Org ETD Custom Module code samples (Create, Delete, List, Get) (#9743) * sample codes for event threat detection custom modules * addressed comments * addressed comments * addressed comments * addressed comments * fix(compute): fixed compute_reservation_create_shared sample and test to use mocked client (#9840) * Fixed sample and test to use mocked client * Fixed code as requested in the comments * feat(compute): add compute instance create replicated boot disk sample (#9735) * Implemented compute_instance_create_replicated_boot_disk sample, created test * Fixed test * Fixed code as requested in the comments * Fixed Util class * Fixed code * feat(compute): add compute consistency group stop replication (#9694) * Implemented compute_consistency_group_create and compute_consistency_group_delete samples, created test * Implemented compute_consistency_group_stop_replication sample * Implemented compute_consistency_group_stop_replication sample * Created test and added needed classes for testing * Fixed test * Moved clean up methods * Added clean up methods for reservations * Fixed clean up method * Fixed clean up method * Added timeout * Reverted not related changes * Reverted not related changes * Reverted not related changes * Reverted not related changes * Fixed code * Split samples for zonal location * Added comments for methods * Fixed comments * feat(secretmanager): add optional ttl to create secret sample (#9889) * feat(secretmanager): add optional ttl to create secret sample * nit: Update secretmanager/src/main/java/secretmanager/CreateSecret.java Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com> * fix(secretmanager): fix comment indentation to resolve linting issues --------- Co-authored-by: Jennifer Davis <[email protected]> Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com> * feat(tpu): add tpu queued resources list sample (#9614) * Changed package, added information to CODEOWNERS * Added information to CODEOWNERS * Added timeout * Fixed parameters for test * Fixed DeleteTpuVm and naming * Added comment, created Util class * Fixed naming * Fixed whitespace * Split PR into smaller, deleted redundant code * Implemented tpu_queued_resources_create, tpu_queued_resources_get, tpu_queued_resources_delete_force and tpu_queued_resources_delete samples, created tests * Implemented tpu_queued_resources_list sample, created test * Fixed test * Fixed tests, deleted cleanup method * Fixed test * Fixed imports * feat(compute): add compute disk create secondary regional sample (#9641) * Implemented compute_disk_create_secondary_regional. created test * Fixed test * Fixed test * Fixed test * Fixed zone * Fixed naming * Fixed spaces * Fixed code * Fixed indentations * Fixed variable * Fixed code * Added cleanup methods * Fixed lint issue * Fixed lint issue * Fixed test * Fixed code * Fixed code * Fixed code * Deleted duplicated assertion * feat(compute): add compute disk create secondary sample. (#9643) * Implemented compute_disk_create_secondary sample, created test * Fixed code * Fixed variable * Fixed code * Merged changes from main * Fixed lint issue * fix(storage): migrate old region all to storagetransfer_transfer_all step 1 (#9917) * fix(job): remove old region create_job (#9914) * feat(compute): attach/ remove snapshot schedule to disk (#9791) * Implemented compute_snapshot_schedule_attach sample, created test * Implemented compute_snapshot_schedule_remove sample, created test * Fixed code * Fixed code as requested in the comments * feat(compute): add compute consistency group clone sample (#9885) * Implemented compute_consistency_group_clone and compute_consistency_group_clone_regional_disk samples, created tests * Fixed naming * feat(compute): add compute instance attach regional disk force sample (#9730) * Implemented compute_instance_attach_regional_disk_force sample, created test * Added clean up method * Fixed comments and parameters * Test order deleted * Fixed code * Fixed code * Fixed code * Increased timeout * Increased timeout * Increased timeout * Fixed code * Fixed code * Fixed code * Fixed naming * feat(compute): add compute disk create secondary custom sample (#9644) * Implemented compute_disk_create_secondary_custom sample, created test * Fixed code * Fixed variable * Fixed code * Fixed whitespace * Fixed whitespace * feat(compute): add compute snapshot schedule create/get/edit/list/delete samples (#9742) * Implemented compute_snapshot_schedule_delete and compute_snapshot_schedule_create samples, created test * Fixed test * Added compute_snapshot_schedule_get sample, created test * Fixed naming * Implemented compute_snapshot_schedule_edit, created test * Fixed naming * Implemented compute_snapshot_schedule_list sample, created test * Cleaned resources * Cleaned resources * Cleaned resources * Cleaned resources * Fixed test * Added comment * Fixed tests * Fixed code * Fixed code as requested in the comments * feat(compute): add compute disk create with snapshot schedule (#9788) * Implemented compute_disk_create_with_snapshot_schedule sample, created test * Fixed code * Fixed code * Fixed test * Fixed code * Fixed code as requested in the comments * Fixed lint issue * Fixed lint issue * Deleted redundant code * feat(tpu): add tpu queued resources time bound sample. (#9617) * Changed package, added information to CODEOWNERS * Added information to CODEOWNERS * Added timeout * Fixed parameters for test * Fixed DeleteTpuVm and naming * Added comment, created Util class * Fixed naming * Fixed whitespace * Split PR into smaller, deleted redundant code * Implemented tpu_queued_resources_create, tpu_queued_resources_get, tpu_queued_resources_delete_force and tpu_queued_resources_delete samples, created tests * Implemented tpu_queued_resources_time_bound sample, created test * Changed zone for tpu * Cleanup resources * Fixed tests * Fixed test * Fixed code as requested in the comments * Fixed code as requested in the comments * fix(job): delete old region tag update_job_with_field_mask (#9940) * feat(job): migrate region tags to include product prefix (#9966) * fix(endpoints): migrate all regions (#9943) * fix: disable flakybot reporting (#9968) * chore(job): remove unused region tags (#9969) * feat(securitycenter): Add Resource SCC Management API Org ETD Custom Module code samples (Update, Get Eff, List Eff, List Desc, Validate) (#9912) * sample codes for event threat detection custom modules * fixed lint * addressed comments * lint fix * addressed comments --------- Co-authored-by: OremGLG <[email protected]> Co-authored-by: eapl.me <[email protected]> Co-authored-by: Тетяна Ягодська <[email protected]> Co-authored-by: Jennifer Davis <[email protected]> Co-authored-by: lovenishs04 <[email protected]> Co-authored-by: alarconesparza <[email protected]> Co-authored-by: Jennifer Davis <[email protected]> Co-authored-by: code-review-assist[bot] <182814678+code-review-assist[bot]@users.noreply.github.com> Co-authored-by: Brian Dorsey <[email protected]>

fix(job): delete old region tag update_job_with_field_mask

2427378

OremGLG requested review from yoshi-approver and a team as code owners December 30, 2024 19:29

ghost reviewed Dec 30, 2024

View reviewed changes

product-auto-label bot added samples Issues that are directly related to samples. api: jobs Issues related to the Cloud Talent Solution Job Search API. labels Dec 30, 2024

blunderbuss-gcf bot assigned minherz Dec 30, 2024

ghost reviewed Dec 30, 2024

View reviewed changes

jobs/v3/src/main/java/com/google/samples/BasicJobSample.java Show resolved Hide resolved

jobs/v3/src/main/java/com/google/samples/BasicJobSample.java Show resolved Hide resolved

ghost reviewed Dec 30, 2024

View reviewed changes

minherz approved these changes Jan 6, 2025

View reviewed changes

OremGLG merged commit 74b655e into GoogleCloudPlatform:main Jan 6, 2025
7 checks passed

OremGLG deleted the garciadiaz-fixit-migrate-step-2-347826098 branch January 6, 2025 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(job): delete old region tag update_job_with_field_mask #9940

fix(job): delete old region tag update_job_with_field_mask #9940

OremGLG commented Dec 30, 2024

ghost left a comment

snippet-bot bot commented Dec 30, 2024

ghost left a comment

ghost Dec 30, 2024

ghost Dec 30, 2024

ghost left a comment

ghost left a comment

ghost left a comment

ghost left a comment

ghost left a comment

ghost left a comment

ghost Dec 30, 2024

minherz left a comment

OremGLG commented Jan 6, 2025

+/**
+ * Updates a job using a field mask to specify which fields to modify.
+ */

-  /** Update a job. */
+/**
+ * Updates a job using a field mask.
+ *
+ * @param jobName The resource name of the job to update.
+ * @param fieldMask A comma-separated list of fields to update.
+ *        See https://cloud.google.com/sdk/gcloud/reference/run/jobs/update#--update-mask for details.
+ * @param jobToBeUpdated The job object containing the updated fields.
+ * @return The updated job.
+ * @throws IOException if an I/O error occurs during the update operation.
+ */

fix(job): delete old region tag update_job_with_field_mask #9940

fix(job): delete old region tag update_job_with_field_mask #9940

Conversation

OremGLG commented Dec 30, 2024

Description

Checklist

ghost left a comment

Choose a reason for hiding this comment

snippet-bot bot commented Dec 30, 2024

ghost left a comment

Choose a reason for hiding this comment

ghost Dec 30, 2024

Choose a reason for hiding this comment

ghost Dec 30, 2024

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost Dec 30, 2024

Choose a reason for hiding this comment

minherz left a comment

Choose a reason for hiding this comment

OremGLG commented Jan 6, 2025