-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: Docker-compose specify platform to get around arm vs amd arch mismatches #241
Conversation
Codecov Report
@@ Coverage Diff @@
## main #241 +/- ##
=======================================
Coverage 76.47% 76.47%
=======================================
Files 92 92
Lines 6758 6758
=======================================
Hits 5168 5168
Misses 1590 1590
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
a couple minor considerations
@@ -3,7 +3,7 @@ | |||
"terraform_version": "0.13.5", | |||
"default_env": "rdev", | |||
"app": "hosted-cellxgene", | |||
"default_compose_env": ".env.ecr", | |||
"default_compose_env_file": ".env.ecr", |
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.
Is this a happy golang related change or was it just broken? If the former, do we also need to update config_version to v2? (per these docs)
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.
If the former, do we also need to update config_version to v2? (per these docs)
we will do those once we've migrated
@@ -3,7 +3,7 @@ | |||
"terraform_version": "0.13.5", | |||
"default_env": "rdev", | |||
"app": "hosted-cellxgene", | |||
"default_compose_env": ".env.ecr", | |||
"default_compose_env_file": ".env.ecr", |
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.
"default_compose_env_file": ".env.ecr", | |
"default_compose_env_file": ".env.ecr", | |
"default_compose_env": ".env.ecr", |
I will keep both for now so that we don't break python
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.
oh woops, maybe I should have been the one to accept this suggestion prior to approval? looks like it merged w/o it.
Make sure we build/run x86 images everywhere.
See https://github.com/chanzuckerberg/czgenepi/pull/999/files
Reviewers
Functional:
Readability:
Changes