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

Move setup back to aqueduct start #197

Merged
merged 5 commits into from
Jul 7, 2022
Merged

Move setup back to aqueduct start #197

merged 5 commits into from
Jul 7, 2022

Conversation

eunice-chan
Copy link
Contributor

@eunice-chan eunice-chan commented Jul 7, 2022

Describe your changes and why you are making these changes

Cannot run post-installation scripts when installing from wheel so moving the code back for now.

aqueduct start now starts with installation which is reflected in the logs like so:

Updating UI version to 0.0.3
Wrote version to file.
Updating server version to 0.0.3
Wrote version to file.
Downloading server binaries.
Operating system is Linux with architecture amd64.
INFO[0000] Executing query: SELECT version,dirty,name FROM schema_version ORDER BY version DESC LIMIT 1; 
...

Related issue number (if any)

Checklist before requesting a review

  • I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
  • I have performed a self-review of my code.
  • I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
  • [N/A] If this is a new feature, I have added unit tests and integration tests.
  • [N/A] I have run the integration tests locally and they are passing.
  • [N/A] All features on the UI continue to work correctly.
  • Added one of the following CI labels:
    • run_integration_test: Runs integration tests
    • skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

@eunice-chan eunice-chan requested a review from cw75 July 7, 2022 01:02
@eunice-chan eunice-chan added the run_integration_test Triggers integration tests label Jul 7, 2022
@eunice-chan eunice-chan added run_integration_test Triggers integration tests and removed run_integration_test Triggers integration tests labels Jul 7, 2022
Copy link
Contributor

@cw75 cw75 left a comment

Choose a reason for hiding this comment

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

@eunice-chan one nit otherwise LGTM

@@ -205,8 +404,7 @@ if __name__ == "__main__":
args = parser.parse_args()

if not os.path.isdir(base_directory):
print("Please install or update aqueduct-ml with `pip install aqueduct-ml`.")
sys.exit(1)
os.makedirs(base_directory)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we remove this since we are already creating this directory in line 179 and 180. Maybe change that to os.makedirs(base_directory) instead?

@eunice-chan eunice-chan added run_integration_test Triggers integration tests and removed run_integration_test Triggers integration tests labels Jul 7, 2022
@cw75 cw75 merged commit 54e4420 into main Jul 7, 2022
@cw75 cw75 deleted the setup branch July 7, 2022 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run_integration_test Triggers integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants