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.

Sign up for GitHub

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

feat: include localhost and canary env as allowed origins for staging backend #71

Merged
merged 4 commits into from
Sep 20, 2021
Merged

feat: include localhost and canary env as allowed origins for staging backend #71

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
31a3bb0
include localhosta and canary env as allowed origins for staging backend
MDunitz Sep 17, 2021
766939a
Merge branch 'main' into dunitz/staging-cors
MDunitz Sep 17, 2021
87edfa4
Update server/eb/app.py
MDunitz Sep 17, 2021
466fc21
Merge branch 'main' into dunitz/staging-cors
tihuan Sep 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions server/eb/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
from flask_talisman import Talisman
from flask_cors import CORS


if os.path.isdir("/opt/python/log"):
# This is the standard location where Amazon EC2 instances store the application logs.
logging.basicConfig(
Expand Down Expand Up @@ -80,8 +79,13 @@ def _before_adding_routes(app, app_config):
web_base_url = server_config.get_web_base_url()
if web_base_url:
web_base_url_parse = urlparse(web_base_url)
allowed_origin = f"{web_base_url_parse.scheme}://{web_base_url_parse.netloc}"
CORS(app, supports_credentials=True, origins=allowed_origin)
allowed_origins = [f"{web_base_url_parse.scheme}://{web_base_url_parse.netloc}"]
if os.getenv('DEPLOYMENT_STAGE') in ["Staging"]:
MDunitz marked this conversation as resolved.
Show resolved Hide resolved
allowed_origins.extend([
"https://canary-cellxgene.dev.single-cell.czi.technology/",
Copy link
Contributor

Choose a reason for hiding this comment

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

The auto-deployed canary env is https://cellxgene.canary.single-cell.czi.technology. Should we use that instead? Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry where are you seeing that? https://canary-cellxgene.dev.single-cell.czi.technology/ is set as the web base url in the config so I think thats what we should be using?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the auto deploy GH flow I was referring to!

https://github.com/chanzuckerberg/single-cell-infra/runs/3547390406?check_suite_focus=true

Screen Shot 2021-09-17 at 2 20 57 PM

I also see canary and canary-hook, so not sure if we only need one or both 😆

https://github.com/chanzuckerberg/single-cell-infra/actions?page=3

Screen Shot 2021-09-17 at 2 21 48 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So canary hook is sent from the explorer repo to the single-cell-infra repo when code is merged into the main-canary branch. Single-cell-infra then kicks off the deployment in the canary step

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh got it, that's why both are needed! Thanks, Madison 🙆‍♂️

So you're saying https://canary-cellxgene.dev.single-cell.czi.technology/ is the right URL, and the smoke test is just pointing to the wrong one (https://cellxgene.canary.single-cell.czi.technology?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea

r"^http://localhost:\d+",
])
CORS(app, supports_credentials=True, origins=allowed_origins)

Talisman(
app,
Expand Down