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

Fix the localdb tests for ARM architecture macOS #4535

Merged
merged 5 commits into from
Mar 9, 2023

Conversation

fuzhaoyuan
Copy link
Contributor

@fuzhaoyuan fuzhaoyuan commented Mar 3, 2023

Fix cBioPortal/cbioportal#10059

Fix the localdb tests for ARM architecture macOS

scripts/e2e.sh Outdated
Comment on lines 58 to 67
if [[ "$(uname -s)" == "Darwin" ]] && [[ "$(sysctl -n machdep.cpu.brand_string)" =~ M.* ]]; then
# if macOS and M-series chip, use images for ARM architecture
export DOCKER_IMAGE_MYSQL=biarms/mysql:5.7
export DOCKER_IMAGE_KEYCLOAK=alemairebe/keycloak:11.0.3
export DOCKER_IMAGE_SESSION_SERVICE=fuzhaoyuan/session-service:0.5.0
else
# else use images for x86_64 architecture
export DOCKER_IMAGE_MYSQL=mysql:5.7
export DOCKER_IMAGE_KEYCLOAK=jboss/keycloak:11.0.3
fi
Copy link
Member

Choose a reason for hiding this comment

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

this feels kinda similar to: https://github.com/cBioPortal/cbioportal-docker-compose/blob/master/init.sh#L7-L11. Wondering if we can just run init.sh in e2e tests as well?

Ok for now tho

Copy link
Contributor Author

@fuzhaoyuan fuzhaoyuan Mar 6, 2023

Choose a reason for hiding this comment

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

Yes of course! I checked the init.sh file but there're some notes I considered:

  1. The first part of init.sh file seems running itself at each sub folder and it not seen at e2e.sh. I'm not sure if it will interfere with the following execution (I do assume not).
  2. If run from yarn command via node, since the current version of node we use (v15) only run through Rosetta translation, the $(arch) command will actually return i386. Therefore I used system check and chip check together to decide the platform.
  3. The keycloak part is not inside init.sh so it needs to be specified in e2e.sh here anyway. So I just add others together.

But that is indeed an option. Let me know if that is a more appropriate way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to go with this approach but it is doing something different because we called init.sh within each sub folder, and the override yaml file doesn't seem to work in this case.

@inodb inodb added the devops label Mar 3, 2023
Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @fuzhaoyuan !

@inodb inodb requested a review from alisman March 9, 2023 15:41
@alisman alisman merged commit fc9298e into cBioPortal:master Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix localdb instance for M1/2 chip machines
3 participants