-
Notifications
You must be signed in to change notification settings - Fork 322
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
[HMA] Fix Create Exchange API Selector #1603
Conversation
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.
Sorry for the incredibly slow review, I've been out on leave for the last week!
Thanks a ton for the help!
The only thing I'm unsure about is the change to the startup script. I thought the devcontainer settings would help with that. When you say it was failing, was this failing a clean rebuild in the created terminal window (and not running manually)?
I can ask @juanmrad since I'll be seeing him in person.
@@ -1,5 +1,5 @@ | |||
#!/bin/bash | |||
set -e | |||
export OMM_CONFIG=/workspace/reference_omm_configs/development_omm_config.py | |||
MIGRATION_COMMAND=1 flask --app OpenMediaMatch.app db upgrade --directory /workspace/src/OpenMediaMatch/migrations | |||
flask --app OpenMediaMatch.app run --debug | |||
MIGRATION_COMMAND=1 flask --app src/OpenMediaMatch.app db upgrade --directory /workspace/src/OpenMediaMatch/migrations |
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.
@juanmrad - can you double check that this is correct? This script should be running with a working directory
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.
Yes it should be running on the working directory. Where you having issues when running the shell script outside of dev container? or where you running it inside?
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.
hey @juanmrad this was failing on the dev container startup
Running the postAttachCommand from devcontainer.json...
[4812 ms] Start: Run in container: /bin/sh -c /workspace/.devcontainer/startup.sh
Error: Could not import 'OpenMediaMatch.app'.
this is after opening vscode in the hasher-matcher-actioner
dir and selecting "Reopen in Container"
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.
Seems like I was able to repro on a whole new build 🤔. Not sure how this may have changed to cause the problem.
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.
Seems like Juan is convinced, lets land it. I'll re-test on windows just to make sure there's not any evil differences between different vscodes.
If you are bored while I'm waiting for Juan's answer, a way to make this less fragile in the future would be to add a simple test (probably a new test) to hit the /ui/ blueprints. |
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.
@aryzle - Can you include more information about the error you were experiencing so we can make sure this is the right fix?
Summary
Fixes #1601 API Select in the Create Exchange form
also fixes issue with startup.sh script, devcontainer was failing before
Test Plan
before
after