Copilot commented on code in PR #986:
URL:
https://github.com/apache/skywalking-banyandb/pull/986#discussion_r2881530320
##########
pkg/fs/remote/gcp/gcs.go:
##########
@@ -90,12 +91,21 @@ func NewFS(p string, cfg *config2.FsConfig) (remote.FS,
error) {
var client *storage.Client
var err error
if cfg.GCP.GCPServiceAccountFile != "" {
- if info, statErr := os.Stat(cfg.GCP.GCPServiceAccountFile);
statErr != nil {
- return nil, fmt.Errorf("credentials file error: %w",
statErr)
+ var info os.FileInfo
+ if info, err = os.Stat(cfg.GCP.GCPServiceAccountFile); err !=
nil {
+ return nil, fmt.Errorf("credentials file error: %w",
err)
} else if info.Mode().Perm() != 0o600 {
return nil, fmt.Errorf("credentials file %s must have
permission 0600", cfg.GCP.GCPServiceAccountFile)
}
- client, err = storage.NewClient(ctx,
option.WithCredentialsFile(cfg.GCP.GCPServiceAccountFile))
+ var credData []byte
+ if credData, err = os.ReadFile(cfg.GCP.GCPServiceAccountFile);
err != nil {
+ return nil, fmt.Errorf("failed to read credentials
file: %w", err)
+ }
+ var creds *google.Credentials
+ if creds, err = google.CredentialsFromJSONWithType(ctx,
credData, google.ServiceAccount, storage.ScopeFullControl); err != nil {
+ return nil, fmt.Errorf("failed to parse credentials:
%w", err)
+ }
+ client, err = storage.NewClient(ctx,
option.WithCredentials(creds))
Review Comment:
The PR description/title suggests this is purely dependency/version updates,
but this hunk changes runtime credential-loading behavior for GCS (reads/parses
JSON and sets explicit scopes). Please either (a) document this functional
change in the PR description or (b) move it into a separate PR so dependency
bumps can be reviewed/rolled back independently.
##########
pkg/fs/remote/gcp/gcs.go:
##########
@@ -90,12 +91,21 @@ func NewFS(p string, cfg *config2.FsConfig) (remote.FS,
error) {
var client *storage.Client
var err error
if cfg.GCP.GCPServiceAccountFile != "" {
- if info, statErr := os.Stat(cfg.GCP.GCPServiceAccountFile);
statErr != nil {
- return nil, fmt.Errorf("credentials file error: %w",
statErr)
+ var info os.FileInfo
+ if info, err = os.Stat(cfg.GCP.GCPServiceAccountFile); err !=
nil {
+ return nil, fmt.Errorf("credentials file error: %w",
err)
} else if info.Mode().Perm() != 0o600 {
return nil, fmt.Errorf("credentials file %s must have
permission 0600", cfg.GCP.GCPServiceAccountFile)
}
- client, err = storage.NewClient(ctx,
option.WithCredentialsFile(cfg.GCP.GCPServiceAccountFile))
+ var credData []byte
+ if credData, err = os.ReadFile(cfg.GCP.GCPServiceAccountFile);
err != nil {
+ return nil, fmt.Errorf("failed to read credentials
file: %w", err)
+ }
+ var creds *google.Credentials
+ if creds, err = google.CredentialsFromJSONWithType(ctx,
credData, google.ServiceAccount, storage.ScopeFullControl); err != nil {
+ return nil, fmt.Errorf("failed to parse credentials:
%w", err)
+ }
+ client, err = storage.NewClient(ctx,
option.WithCredentials(creds))
Review Comment:
`golang.org/x/oauth2/google` doesn’t provide `CredentialsFromJSONWithType`
(and `google.ServiceAccount` isn’t a symbol in that package), so this won’t
compile. Use `google.CredentialsFromJSON(ctx, credData, storage.Scope...)` (or
revert to `option.WithCredentialsFile` / `option.WithCredentialsJSON`) and, if
you need to enforce service-account JSON, validate the `"type"` field yourself
before creating the client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
