Copy the existing gcloud config directory if it exists. #1761
Conversation
igorpeshansky
force-pushed
the
igorpeshansky-gcloud-config-context
branch
2 times, most recently
from
59f822c to
dd962fb
Compare
This is needed to run integration tests locally.
igorpeshansky
force-pushed
the
igorpeshansky-gcloud-config-context
branch
from
dd962fb to
47be625
Compare
integration_test/gce/gce_testing.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| sfi, err := os.Stat(currentConfigDir) |
There was a problem hiding this comment.
this is a lot of code! Stackoverflow says that go 1.23 will likely get a function to do (approximately) this, but in the meantime, why not just runCommand() a "cp --recursive" command? Do we really need to copy only these relevantFiles or something?
There was a problem hiding this comment.
Yes, I saw golang issue 62484, but we're not on 1.23 yet… The good news is that, when we do upgrade, it'll look like this:
func SetupGcloudConfigDir(ctx context.Context, directory string) error {
currentConfigDir, err := getGcloudConfigDir(ctx)
if err != nil {
return err
}
err = os.CopyFS(directory, os.DirFS(currentConfigDir))
if os.IsNotExist(err) {
return nil
}
return err
}Regarding using cp, this was my first thought too, but, sadly, this needs to work on Windows as well.
There was a problem hiding this comment.
why does this need to work on windows as well? I'm pretty sure it doesn't... even when testing Windows VMs, the kokoro worker is still a Linux machine, described here: https://github.com/GoogleCloudPlatform/ops-agent/blob/6cbd2f752094bdeb44a54ac7b96300dc82774e05/kokoro/scripts/test/go_test.Dockerfile
There was a problem hiding this comment.
You're right, it doesn't need to work on Windows after all. I didn't think of that initially. That said, once we've upgraded to Go 1.23, the code to invoke os.CopyFS (above) would be ~same as the code to invoke runCommand(…, []string{"cp", "--recursive"}, …), and, IMO, less hacky. But I'll rewrite using "cp -r" if you insist.
There was a problem hiding this comment.
I think I was proposing:
- cp -r for now, with a TODO I guess
- replace that with os.CopyFS when it becomes available, because it's less hacky
This saves us about 50+ lines of code of hand-rolled directory copying code. Like for example, defer in.Close() drops errors from closing files, but we don't need to hash that out if you are just calling cp -r. So yeah I think cp -r is worth doing, even if only to save time going back and forth in reviews.
igorpeshansky
force-pushed
the
igorpeshansky-gcloud-config-context
branch
from
8e2e5e1 to
69bde8e
Compare
integration_test/gce/gce_testing.go
Outdated
| return err | ||
| } | ||
| // TODO: Replace with os.CopyFS() once available. | ||
| if out, err := runCommand(ctx, log.New(io.Discard, "", 0), nil, []string{"cp", "-r", filepath.Join(currentConfigDir, "."), filepath.Join(directory, ".")}, nil); err != nil { |
There was a problem hiding this comment.
why are you adding a /. on the end of currentConfigDir and directory?
Seems unnecessary
There was a problem hiding this comment.
The /. on the source directory is to ensure that only the contents are copied (https://askubuntu.com/a/1151667). The /. on the target directory is to ensure that it exists (and is a directory).
Description
Copy the existing
gcloudconfig directory if it exists. This will avoid errors in situations where testing relies on this configuration, e.g., when running integration tests locally with cached credentials. A follow-on to #1750.Related issue
b/352054649
How has this been tested?
Integration tests.
Checklist: