Merge #290 related changes from main to epic/issue-285-client-v2 - #293
Merged
Conversation
kyasbal
requested review from
RyuSA,
jyane,
kkuchima and
yfuruyama
as code owners
October 6, 2025 08:20
Contributor
Summary of ChangesHello @kyasbal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request merges a set of already reviewed changes from the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request primarily renames PreviousTaskResult to CacheableTaskResult across the codebase, which improves clarity. The changes are mostly straightforward refactoring. However, I've identified a couple of issues where errors from NewClient() are being swallowed instead of propagated, which could mask underlying problems. I've also pointed out a minor typo in a function parameter. Please address these points to ensure robustness and maintainability.
| client, err := googlecloudapi.DefaultGCPClientFactory.NewClient() | ||
| if err != nil { | ||
| return inspectiontaskbase.PreviousTaskResult[*googlecloudk8scommon_contract.AutocompleteClusterNameList]{}, nil | ||
| return inspectiontaskbase.CacheableTaskResult[*googlecloudk8scommon_contract.AutocompleteClusterNameList]{}, nil |
Contributor
There was a problem hiding this comment.
The error from googlecloudapi.DefaultGCPClientFactory.NewClient() is being swallowed. It should be propagated. This can hide important issues like authentication or configuration problems.
return inspectiontaskbase.CacheableTaskResult[*googlecloudk8scommon_contract.AutocompleteClusterNameList]{}, err| client, err := googlecloudapi.DefaultGCPClientFactory.NewClient() | ||
| if err != nil { | ||
| return inspectiontaskbase.PreviousTaskResult[*googlecloudk8scommon_contract.AutocompleteClusterNameList]{}, nil | ||
| return inspectiontaskbase.CacheableTaskResult[*googlecloudk8scommon_contract.AutocompleteClusterNameList]{}, nil |
Contributor
There was a problem hiding this comment.
The error from googlecloudapi.DefaultGCPClientFactory.NewClient() is being swallowed. It should be propagated. This can hide important issues like authentication or configuration problems.
return inspectiontaskbase.CacheableTaskResult[*googlecloudk8scommon_contract.AutocompleteClusterNameList]{}, err
Comment on lines
+38
to
39
| func NewCachedTask[T any](taskID taskid.TaskImplementationID[T], depdendencies []taskid.UntypedTaskReference, f func(ctx context.Context, prevValue CacheableTaskResult[T]) (CacheableTaskResult[T], error), labelOpt ...coretask.LabelOpt) coretask.Task[T] { | ||
| return coretask.NewTask(taskID, depdendencies, func(ctx context.Context) (T, error) { |
Contributor
There was a problem hiding this comment.
There's a typo in the parameter name depdendencies. It should be dependencies.
Suggested change
| func NewCachedTask[T any](taskID taskid.TaskImplementationID[T], depdendencies []taskid.UntypedTaskReference, f func(ctx context.Context, prevValue CacheableTaskResult[T]) (CacheableTaskResult[T], error), labelOpt ...coretask.LabelOpt) coretask.Task[T] { | |
| return coretask.NewTask(taskID, depdendencies, func(ctx context.Context) (T, error) { | |
| func NewCachedTask[T any](taskID taskid.TaskImplementationID[T], dependencies []taskid.UntypedTaskReference, f func(ctx context.Context, prevValue CacheableTaskResult[T]) (CacheableTaskResult[T], error), labelOpt ...coretask.LabelOpt) coretask.Task[T] { | |
| return coretask.NewTask(taskID, dependencies, func(ctx context.Context) (T, error) { | |
kyasbal
added a commit
that referenced
this pull request
Oct 21, 2025
…using official SDK (#324) * [fix#285 1/N] feat(api): introduce v2 client factory (#286) * feat(api): introduce v2 client factory Introduces a new, more flexible factory for creating Google Cloud service clients eventually replacing the current clients calling REST endpoint for Google Cloud API. Key features include: - A `ClientFactory` to centralize client creation for services like Container, GKE Hub, Composer, and Logging. - A `ResourceContainer` interface to represent different resource scopes (e.g., a project, an organization ...etc) - A set of `ClientFactoryOptions` to configure authentication using service account keys or token sources, with the ability to apply them per-project. * fix problems pointed by gemini-code-assist * Merge #290 related changes from main to epic/issue-285-client-v2 (#293) * Rename inspectiontaskbase.PreviousTaskResult to CachableTaskResult (#290) * Fix typo on #290 (#292) * [fix#285 2/N] feat(api): implement OAuth server for api client v2 (#287) * [fix#285 2/N] feat(api): added oauth server implementation for client v2 * Fix issues pointed out by gemini-code-assist * [feat #285 3/N] Implemented a function to convert loggingpb.LogEntry to structured.Node (#289) * [fix #285 3/N] feat(api): Implement converter function from the log client to KHI's log type * fixed issues pointed by gemini-code-assist * Merge main change into epic/issue-285-api-cleint-v2 (#304) * Rename inspectiontaskbase.PreviousTaskResult to CachableTaskResult (#290) * Fix typo on #290 (#292) * feat(server): Add ServerFactory type to configure the gin.Engine (#288) * feat(server): Add ServerFactory type to configure the gin.Engine instance easier in the initialization phase Several KHI feature sometimes needs to register API handler on gin.Engine but it was difficult before. This makes the initialization code under cmd/kubernetes-history-inspector harder. This change introduced a new type ServerFactory used for instancing a new gin.Engine instance. This type receives Option parameters to extend the server instance. This allows user to extend its behavior just by adding new init() function on cmd/kubernetes-history-inspector or each task package folders. * fix points pointed out by gemini-code-review * [feat #285 5-7-1/N] Adding a progress reportable log client with the v2 API client (#301) * [feat #285 4/N] Add google cloud common task injecting Google Cloud API client (#294) * [feat #285 4/N] Add google cloud common task injecting Google Cloud API client * fixed issues pointed out by gemini-code-assist * Rewrite document about authentication method using ADC (#306) * Rewrite document about authentication method using ADC * Fix issues pointed out by gemini-code-assist * Removed unused part * Format the command to be multi lined for better readability * [feat #285 5-1/N] Replace API client to v2 for GKE cluster name auto completion (#295) * [feat #285 5-1/N] Replace API client to v2 for GKE cluster name auto completion * fix issues pointed out by gemini-code-assist * [feat #285 5-2/N] Replace API client to v2 for Cloud Composer cluster name auto completion (#296) * [feat #285 5-2/N] Replace API client to v2 for CloudComposer cluster name auto completion * fix issues pointed out by gemini-code-assist * [feat #285 5-3/N] Replace API client to v2 for GDC baremetal cluster name auto completion (#297) * [feat #285 5-3/N] Replace API client to v2 for GDC Baremetal cluster name auto completion * fix issues pointed out by gemini-code-assist * Added tests for clusterlistfetcher * [feat #285 5-4/N] Replace API client to v2 for GDC VMware cluster name auto completion (#298) * [feat #285 5-4/N] Replace API client to v2 for GDC VMWare cluster name auto completion * fix issues pointed out by gemini-code-assist * added tests for clusterlistfetcher * [feat #285 5-5/N] Replace API client to v2 for GKE on AWS cluster name auto completion (#299) * [feat #285 5-5/N] Replace API client to v2 for GKE on AWS cluster name auto completion * fix issues pointed out by gemini-code-assist * [feat #285 5-6/N] Replace API client to v2 for GKE on Azure cluster name auto completion (#300) * [feat #285 5-6/N] Replace API client to v2 for GKE on Azure cluster name auto completion * fix issues pointed out by gemini-code-assist * Make ClientFactory extensible for adding caller options to specific client (#307) * Merge main branch into epic/issue-285-api-client-v2 (#313) * Rename inspectiontaskbase.PreviousTaskResult to CachableTaskResult (#290) * Fix typo on #290 (#292) * feat(server): Add ServerFactory type to configure the gin.Engine (#288) * feat(server): Add ServerFactory type to configure the gin.Engine instance easier in the initialization phase Several KHI feature sometimes needs to register API handler on gin.Engine but it was difficult before. This makes the initialization code under cmd/kubernetes-history-inspector harder. This change introduced a new type ServerFactory used for instancing a new gin.Engine instance. This type receives Option parameters to extend the server instance. This allows user to extend its behavior just by adding new init() function on cmd/kubernetes-history-inspector or each task package folders. * fix points pointed out by gemini-code-review * fix lint related minor issues (#309) * fix lint related minor issues * exclude *.go files under `**/test` folder to be evaluated by "no-testutil-in-non-test-files" rule. * Use non containerlized golangci-lint when it's available to run them faster. * fix issues pointed by gemini-code-assist * [feat #285 5-7-2/N] Adding a progress reportable log client with partitioning time duration (#302) * [feat #285 5-7-2/N] Adding a progress reportable log client with partitioning time duration * fix issues pointed out by gemini-code-assist * Use compute API client to get the list of available regions (#311) * Use compute API client to get the list of available regions * fix issues pointed out by gemini-cloud-assist * [feat #285 5-7-3] Migrate current query_base task implementation to use the api client v2 (#308) * [feat #285 5-7-3/N] Replace current log fetch base task to use the new types using SDK based client * fixed issues pointed by gemini-code-assist * Update epic/issue-285-api-client-v2 to track the latest main branch (#314) * Rename inspectiontaskbase.PreviousTaskResult to CachableTaskResult (#290) * Fix typo on #290 (#292) * feat(server): Add ServerFactory type to configure the gin.Engine (#288) * feat(server): Add ServerFactory type to configure the gin.Engine instance easier in the initialization phase Several KHI feature sometimes needs to register API handler on gin.Engine but it was difficult before. This makes the initialization code under cmd/kubernetes-history-inspector harder. This change introduced a new type ServerFactory used for instancing a new gin.Engine instance. This type receives Option parameters to extend the server instance. This allows user to extend its behavior just by adding new init() function on cmd/kubernetes-history-inspector or each task package folders. * fix points pointed out by gemini-code-review * fix lint related minor issues (#309) * fix lint related minor issues * exclude *.go files under `**/test` folder to be evaluated by "no-testutil-in-non-test-files" rule. * Use non containerlized golangci-lint when it's available to run them faster. * fix issues pointed by gemini-code-assist * Made context used in inspection graph extensible from InspectionServer type (#305) * Made context used in inspection graph extensible from InspectionServer type * fix issues pointed out by gemini-code-assist * Enabled OAuth option with the client v2 (#315) * Enabled OAuth option with the client v2 * fix issues pointed by gemini-code-review * Complete replacement of API client (#316) * Complete replacement of API client * fix issue pointed out by gemini-code-assist * Rename API package name from `googlecloudv2` to `googlecloud` (#320) * Implemented CallOptionInjector to inject options required for both clients commonly (#322) * Add a type to inject options for each API calls Added CallOptionInjector to provide an option before calling API. This is implemented for providing header or grpc.Header not supported by gax. * fix the CI failing issue due to lack of ADC cred * Merge main into epic/issue-285-api-client-v2 (#323) * Implemented `--access-token` parameter again just for keeping compatibility with the older versions (#325) * Implemented `--access-token` parameter again just for keeping compatibility with the older versions The parameter `--access-token` is now deprecated after we started supporting Application Default Credentials(ADC), but we wrote many external documents using the parameter. We are going to keep this parameter just only for near future to make our users to migrate their authetication method to ADC. * fix problems pointed by gemini-code-assist --------- Signed-off-by: kyasbal <ikakeru@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This is already reviewed on main branch. I'll merge them on epic/issue-285-api-client-v2 just after creating this PR.