packer_test: make PluginTestDir a structure #13163
Conversation
lbajolet-hashicorp
added
the
tech-debt
packer_test/common/plugin.go
Outdated
| } | ||
| t.Fatalf("failed to prepare temporary plugin directory %q: %s", pluginTempDir, err) | ||
| t.Fatalf("failed to install plugins to temporary plugin directory %q: %s", ps.dirPath, err) |
There was a problem hiding this comment.
Does it makes sense to use the accessor method ps.Dir() here?
There was a problem hiding this comment.
I believe it does yes! I'll have to double-check why I didn't use it in the first place, but I assume this would be safe to use
| return ps.dirPath | ||
| } | ||
|
|
||
| func (ps *PluginDirSpec) Cleanup() { |
There was a problem hiding this comment.
Does it make sense to check that ps.dirPath is not an empty string?
There was a problem hiding this comment.
Yeah can't hurt
| pluginDir, cleanup := ts.MakePluginDir() | ||
| defer cleanup() | ||
| pluginDir := ts.MakePluginDir() | ||
| defer pluginDir.Cleanup() |
There was a problem hiding this comment.
My only nit here is that by making this a method on pluginDir as opposed to an anonymous returned function you can easily change the value of pluginDir.dirPath and remove some other directory.
There was a problem hiding this comment.
That's a valid concern, but I wouldn't think this is a huge problem since the dir argument is private, and therefore can only be changed from within the package it's defined in. Since tests are in a separate package, it's not really something you can accidentally change
There was a problem hiding this comment.
Yeah that's fair. This concern is solely from this package as I saw a reference to ps.dirPath up above.
It is not a show stopper TBH honest. I called it out because we sometimes change the plugin directory for testing purposes.
In order for the creation of a temporary directory to install plugins into to be simpler to understand and use, we change how the directory is created, cleaned-up, and installs plugins into. Now, instead of a tuple of a string (path) and a cleanup function, we return a structure that comprises the test suite, and the temporary directory, along with methods to handle those steps independently.
lbajolet-hashicorp
force-pushed
the
PluginTestDir_struct
branch
from
4712fa7
to
bf34bf4
Compare
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
In order for the creation of a temporary directory to install plugins into to be simpler to understand and use, we change how the directory is created, cleaned-up, and installs plugins into.
Now, instead of a tuple of a string (path) and a cleanup function, we return a structure that comprises the test suite, and the temporary directory, along with methods to handle those steps independently.