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

Add cleanup #2

Merged
merged 2 commits into from
May 3, 2024
Merged

Add cleanup #2

merged 2 commits into from
May 3, 2024

Conversation

webbnh
Copy link

@webbnh webbnh commented May 2, 2024

Hi Ed, here's another PR with some updates for your PR arcalot#61. The big deal is reworking the cleanup in your test to make it more robust, but, while I was at it, I folded in the items from my feedback to your latest changes. Here's what I've got for you:

  • Moved the code which kills the Podman service process and removes the Podman connection into cleanup functions called by t.Cleanup(), so that, if the test crashes or otherwise misbehaves, the cleanup will still be done.
  • Using the cleanup handlers allows the code to use a common call to the test helper function regardless of whether it opts to create a local service or whether it uses the default one, which makes the code a little more graceful.
  • Moved the code to create the local service into a helper function to appease the linter. Also, added lint directives to the exec.Command() calls and added some annotations to them.
  • Changed NewCliWrapper() to take a pointer to the connection string instead of the value: this allows the caller to pass the configuration value straight through without having to conditionally instantiate an otherwise useless empty string.
  • Tweaked NewCliWrapper() to use append() to add the connection option instead of instantiating a new slice.
  • Renamed a few things in the test code.
  • Changed the image-exists helper test function to take the connection as a pointer instead of a string, so that it can pass the value straight through to NewCliWrapper() and fixed the callers.

So, it now passes the linters and the tests.

Thanks for your help with this!

Use cleanup handlers to tear down the temporary Podman API service.
Pass the connection via a pointer to a string to make handling
  null values more graceful.
Tweak some names and text.
Pick lint.
Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

Sure, so long as it's never possible to have two instances running. How are GitHub Actions isolated??? 🦈

internal/cliwrapper/cliwrapper_test.go Outdated Show resolved Hide resolved
// Clean up
if err := tmpPodmanSocketCmd.Process.Kill(); err != nil {
t.Fatalf("Failed to kill temporary socket")
connectionName = sockDir

Choose a reason for hiding this comment

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

This should return something like ${TMPDIR}/arcaflow-engine-deployer-podman-test-<nnn>. Is that really what you want to use for the connection name? (Does podman even guarantee it accepts slashes in the connection name?) I'd think you'd want filepath.Base(filepath.Dir(sockDir))...

(Although ... podman does seem to accept slashes in the name, so I guess it'll work. It still feels odd, but I suppose there's no need to overcomplicate this...)

Copy link
Author

Choose a reason for hiding this comment

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

I checked that Podman would accept the slashes, and then I checked that the test worked, so I so I figured it was good enough, especially since, presumably, no one but the test itself would ever see or use the connection name. (And, it was way simpler than parsing out the directory base name. 🙂)

@ewchong ewchong merged commit a289a3d into ewchong:main May 3, 2024
@webbnh webbnh deleted the add-cleanup branch June 10, 2024 20:06
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.

3 participants