-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/export experiments #15
Conversation
krebsni
commented
May 27, 2022
•
edited
Loading
edited
- refactor export/import experiments into long-running tasks
- add filter for experiment export
…ved to types in the future)
…ved to types in the future)
…ipping not implmented yet
…er... seems to be bug in ballerina
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to investigate the path join issues before we can merge this
modules/database/export.bal
Outdated
# | ||
# + test - some config field | ||
public type ExperimentExportConfig record {| | ||
string test; // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is there TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a future PR export config needs to be added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then clarify the TODO comment (just TODO is not enough...)
modules/database/export.bal
Outdated
result = check os:exec({value: "powershell", arguments: ["Compress-Archive", "-Update", jsonPath, zipPath]}); | ||
} else { | ||
return error("Unsupported operating system! At the moment, we support 'linux' and 'windows' for importing/exporting experiments. Please make sure to properly specify the os env var or config entry."); | ||
if !os.toLowerAscii().includes("linux") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above, also check if else needs {}
if it is meant as else-if.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case it's not linux but linux-like it may be worth printing an error and still trying to run the exec. If it fails an exception should be raised. Therefore the if within the else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor change left.