[RSDK-9498] Search for meta.json File in Top Level of Online Module #4689
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
config/module.go
Outdated
| @@ -282,7 +282,13 @@ func (m *Module) FirstRun( | |||
| logger.Debug("no first run script specified, skipping first run") | |||
| return nil | |||
| } | |||
| relFirstRunPath, err := utils.SafeJoinDir(unpackedModDir, meta.FirstRun) | |||
| var firstRunTopLevelDir string | |||
There was a problem hiding this comment.
Before, meta.FirstRun was used relative to the directory specified by unpackedModDir. In the case that the module has a type of registry, this would just be the parent directory of the module's entry point executable - not necessarily the top level of its directory. Here, I've changed it so that if moduleWorkingDirectory is not the empty string (which is equivalent to saying that the module's type is registry and the VIAM_ROOT_MODULE environment variable has been set), then the module's top level directory is used to find and execute the specified "first run" script.
There was a problem hiding this comment.
why not just have moduleWorkingDirectory get returned all the time?
There was a problem hiding this comment.
Good point, this will clean up the code a bit.
config/module.go
Outdated
| return meta, "", err | ||
| } | ||
|
|
||
| if m.Type == ModuleTypeRegistry { |
There was a problem hiding this comment.
Before, this function would automatically return an error and no meta information when m.Type == ModuleTypeRegistry
There was a problem hiding this comment.
I feel like this case should be at the top then, and also only check in the moduleWorkingDirectory.
Since if m.Type == ModuleTypeRegistry then this will go through the !localNonTarball branch automatically
config/module.go
Outdated
| return nil, err | ||
| } | ||
| _, err = os.Stat(metaPath) | ||
| if err == nil { |
There was a problem hiding this comment.
have the conditional be err != nil so we can early return
config/module.go
Outdated
| @@ -381,38 +387,75 @@ func (m *Module) FirstRun( | |||
| // 1. if there is a meta.json in the exe dir, use that, except in local non-tarball case. | |||
There was a problem hiding this comment.
the comment should be rewritten to reflect the current state of things.
| @@ -381,38 +387,75 @@ func (m *Module) FirstRun( | |||
| // 1. if there is a meta.json in the exe dir, use that, except in local non-tarball case. | |||
| // 2. if this is a local tarball and there's a meta.json next to the tarball, use that. | |||
| // Note: the working directory must be the unpacked tarball directory or local exec directory. | |||
| func (m Module) getJSONManifest(unpackedModDir string) (*JSONManifest, error) { | |||
| func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*JSONManifest, string, error) { | |||
There was a problem hiding this comment.
would like to see a test for this
| @@ -267,7 +267,7 @@ func (m *Module) FirstRun( | |||
|
|
|||
| // Load the module's meta.json. If it doesn't exist DEBUG log and exit quietly. | |||
| // For all other errors WARN log and exit. | |||
| meta, err := m.getJSONManifest(unpackedModDir) | |||
| meta, moduleWorkingDirectory, err := m.getJSONManifest(unpackedModDir, env) | |||
There was a problem hiding this comment.
want to see an integration test around this whole function
There was a problem hiding this comment.
Do you mean around getJSONManifest()? or around the FirstRun() which calls getJSONManifest()?
There was a problem hiding this comment.
around FirstRun() at the very least, would recommend trying to unit test getJSONManifest() as well
config/module.go
Outdated
| return meta, "", err | ||
| } | ||
|
|
||
| if m.Type == ModuleTypeRegistry { |
There was a problem hiding this comment.
I feel like this case should be at the top then, and also only check in the moduleWorkingDirectory.
Since if m.Type == ModuleTypeRegistry then this will go through the !localNonTarball branch automatically
config/module.go
Outdated
| @@ -282,7 +282,13 @@ func (m *Module) FirstRun( | |||
| logger.Debug("no first run script specified, skipping first run") | |||
| return nil | |||
| } | |||
| relFirstRunPath, err := utils.SafeJoinDir(unpackedModDir, meta.FirstRun) | |||
| var firstRunTopLevelDir string | |||
There was a problem hiding this comment.
why not just have moduleWorkingDirectory get returned all the time?
config/module.go
Outdated
| if !ok { | ||
| return nil, "", errors.Errorf("VIAM_MODULE_ROOT not set. Failed to find meta.json in executable directory %s", unpackedModDir) | ||
| } | ||
| return nil, "", errors.Errorf("failed to find meta.json. Searched in executable directory %s and path set by VIAM_MODULE_ROOT %s", |
There was a problem hiding this comment.
I think yes. As this function is currently written, we only search for the meta.json file in two locations 1) the top level directory of the online module and 2) in the same sub-directory of the module's executable. If the meta.json lives anywhere else, it won't be found - even if the VIAM_MODULE_ROOT env. variable is set. I could be wrong re: how modules are unpacked/sourced/retrieved.
There was a problem hiding this comment.
I ask because I'm pretty sure I saw both terminal ok and !ok cases at the time
viambot
added
safe to test
|
also adding @benjirewis |
viambot
removed
the
safe to test
viambot
added
the
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
Search for a registry module's meta.json file in the top level of the module and in the parent directory of its executable entry point.
Testing
Running the robot linked in the Jira ticket no longer outputs the WARN log listed.
first_run.shis successfully sourced and executed, and adist.first_run_succeededfile is created.Note: This has only been tested with a registry module, not a local module.