Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing: Use simpler logger types #1501

Merged
merged 8 commits into from
Nov 20, 2023
Merged

Conversation

martijnvans
Copy link
Contributor

@martijnvans martijnvans commented Nov 7, 2023

Description

The goal of this PR was to make gce.RunScriptRemotely more user-friendly by replacing its logging.DirectoryLogger argument with a plainer log.Logger argument. This removes the requirement that all transitive callsites to this function (of which there are a lot) pass around a logging.DirectoryLogger, allowing them to skip A LOT of calls to .ToMainLog() all over the place. [fun fact: ops_agent_test.go is more than 1% smaller due to this PR.] In addition to being cleaner to look at, it's also better to decide which log file to write to in a small number of places, instead of having every single callsite to the logger make the decision separately to write to the main log.

For some perspective, there were 170 calls to .ToMainLog() in ops_agent_test before this PR, and only 3 calls to .ToFile(), indicating that the vast majority of the time, just using the main log is fine. [And if we wanted to, I could argue that the remaining 3 calls are also unnecessary with some tweaks, but that's not a concern for me here.]

This PR is in the spirit of #1238, in which I realized that sometimes it's inconvenient to pass around a DirectoryLogger and started the process of limiting its use to the (few) places that actually need it.

A downside (?) to this PR is that a small amount of "noise" that was previously stowed away in file_uploads.txt for Windows tests is now going to appear in the main log. However, it's really not a lot of noise. For a single call to RunScriptRemotely, we'll have this much additional text in main_log.txt:

2023/11/03 20:32:43 Running command remotely: Read-GcsObject -Force -Bucket "stackdriver-test-143416-untrusted-file-transfers" -ObjectName "github-test-20231103-1b83c-87790e19-9413-41a9-97db-456412121ff6/C:\df21c16e-d772-486b-837e-c40aa906f459.ps1" -OutFile "C:\df21c16e-d772-486b-837e-c40aa906f459.ps1"
2023/11/03 20:32:47 exit code: 0
2023/11/03 20:32:47 stdout+stderr: #< CLIXML
<Objs Version="1.1.0.1" xmlns="http://schemas.microsoft.com/powershell/2004/04"><Obj S="progress" RefId="0"><TN RefId="0"><T>System.Management.Automation.PSCustomObject</T><T>System.Object</T></TN><MS><I64 N="SourceId">1</I64><PR N="Record"><AV>Preparing modules for first use.</AV><AI>0</AI><Nil /><PI>-1</PI><PC>-1</PC><T>Completed</T><SR>-1</SR><SD> </SD></PR></MS></Obj></Objs>
2023/11/03 20:32:47 Uploading file finished with err=<nil>

And the last line of this noise won't be printed anyway unless there are errors (after this PR, see minor change to gce_testing.go).

After this PR, there is only one dependency of gce_testing.go on DirectoryLogger, and that is just gce.SetupLogger(), which could be moved into the logging package (in some future PR).

Related issue

no bug, cleanup

How has this been tested?

Automated tests only

Checklist:

  • Unit tests
    • Unit tests do not apply.
    • Unit tests have been added/modified and passed for this PR.
  • Integration tests
    • Integration tests do not apply.
    • Integration tests have been added/modified and passed for this PR.
  • Documentation
    • This PR introduces no user visible changes.
    • This PR introduces user visible changes and the corresponding documentation change has been made.
  • Minor version bump
    • This PR introduces no new features.
    • This PR introduces new features, and there is a separate PR to bump the minor version since the last release already.
    • This PR bumps the version.

@martijnvans martijnvans marked this pull request as ready for review November 14, 2023 16:04
@@ -3898,12 +3907,11 @@ func installGolang(ctx context.Context, logger *logging.DirectoryLogger, vm *gce
Start-Process msiexec.exe -ArgumentList "/i","golang.msi","/quiet" -Wait `, goVersion, goArch)
} else {
installCmd = fmt.Sprintf(`
set -e
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this set -e is superfluous when the script is only one line long, and it's also superfluous when using RunRemotely. so I got rid of it.

gsutil cp \
"gs://stackdriver-test-143416-go-install/go%s.linux-%s.tar.gz" - | \
tar --directory /usr/local -xzf /dev/stdin`, goVersion, goArch)
sudo tar --directory /usr/local -xzf /dev/stdin`, goVersion, goArch)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RunScriptRemotely runs with "sudo" while RunRemotely does not, so I had to stick a sudo on here.

@martijnvans martijnvans merged commit 87532b3 into master Nov 20, 2023
9 checks passed
@martijnvans martijnvans deleted the martijnvans-use-simpler-logger branch November 20, 2023 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants