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

Implement reading all wasm module roots in machine locator [NIT-2444] #2254

Merged
merged 6 commits into from
Apr 29, 2024

Conversation

anodar
Copy link
Contributor

@anodar anodar commented Apr 23, 2024

No description provided.

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Apr 23, 2024
@anodar anodar changed the title Implement reading all wasm module roots in machine locator Implement reading all wasm module roots in machine locator [NIT-2444] Apr 23, 2024
@anodar anodar marked this pull request as ready for review April 23, 2024 11:11
@anodar anodar requested a review from tsahee April 23, 2024 15:55
Base automatically changed from nitro-pubsub to master April 23, 2024 23:31
moduleRoots[moduleRoot] = true
if file.Name() == "latest" {
latestModuleRoot = moduleRoot
rootPath = dir
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Only include dirs if their name is either "latest" or the wasmMduleRoot - otherwise locator will not be able to find it
see GetMachinePath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that exactly what I'm doing already ? Iterating all folders, checking if there's module-root.txt then reading file content (which is same as directory name if it's not "latest"), then adding to moduleRoots.

Or am I missing something ?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you have a file "foobar/module-root.txt" - you cannot use this module-root because the locator won't be able to find "foobar" dir later. We can change that in GetMachinePath.. but easier to just not include it in the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, ptal.

@anodar anodar requested a review from tsahee April 29, 2024 10:55
@anodar anodar enabled auto-merge April 29, 2024 16:40
Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

LGTM.
One comment but I plan to fix it in a separate PR

moduleRoots[moduleRoot] = true
if file.Name() == "latest" {
latestModuleRoot = moduleRoot
rootPath = dir
Copy link
Contributor

Choose a reason for hiding this comment

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

rootpath=dir doesn't belong in the latest "if". machines might not have a "latest" and it'd still be correct

@anodar anodar merged commit c74d874 into master Apr 29, 2024
8 checks passed
@anodar anodar deleted the nitro-machine-locator branch April 29, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants