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

[RW-10031] Remove deprecated Google auth API usage #421

Closed

Conversation

Peter-Lavigne
Copy link

This is the minimum changes necessary to prevent any issues when the google library is deprecated and prevent contention between AoU’s periodic refresh code and the extension’s refreshing.

See details in this thread.

See my manual testing process in the description of the first commit.

This is my first contribution to terra-docker so let me know if there's anything I should do beyond following this guide.

@Peter-Lavigne Peter-Lavigne requested a review from rtitle May 19, 2023 15:46
@jdcanas jdcanas self-requested a review May 19, 2023 15:48
@Peter-Lavigne
Copy link
Author

I'm getting the following error in CI:

error: pathspec 'cromshell_2.0' did not match any file(s) known to git

This seems unrelated to my changes. What should I do about this?

@@ -1,4 +1,4 @@
FROM us.gcr.io/broad-dsp-gcr-public/terra-jupyter-gatk:2.2.11
FROM us.gcr.io/broad-dsp-gcr-public/terra-jupyter-gatk:2.2.12
Copy link
Contributor

Choose a reason for hiding this comment

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

I think rebuild terra-base automattly upgrades Cromshell-beta to cromshell per
https://github.com/broadinstitute/cromshell/releases/tag/2.0.0
Can you help to verify, and if true also remove the alias Cromshell-beta code?

Copy link
Author

Choose a reason for hiding this comment

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

This PR covers that change.

Copy link
Author

Choose a reason for hiding this comment

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

Once that PR is released I'll bump the version and rebase this branch.

How I validated this:

1. I installed the image:

`docker pull us.gcr.io/broad-dsp-gcr-public/terra-jupyter-base`

2. I ran the command as a root user:

`docker run --user root -d -p 9000:8000 -v ~/src/terra-docker:/home/jupyter -it us.gcr.io/broad-dsp-gcr-public/terra-jupyter-base --allow-root`

3. I opened the Jupyter terminal and ran the following commands:

`cd terra-jupyter-base/custom`
`jupyter nbextension install google_sign_in.js --symlink`
`jupyter nbextension enable google_sign_in`

4. I opened Jupyter and observed that the extension was loaded

5. By manually setting `params.googleClientId = "fake"` in the extension's `init` function,
   I was able to compare the behavior with and without this commit.

5a. Without this commit, I observed (via the network tab) the auth API
    being (unsuccessfully) called.

5b. With this commit, that call is no longer made.
To create this commit, I ran the following command:
`amm updateVersions.sc --updatedImage "terra-jupyter-base" --updatedImageReleaseNote "Removed deprecated google auth usage"`
@Peter-Lavigne Peter-Lavigne force-pushed the peterlavigne/rw-10031-remove-deprecated-gapi-usage branch from 3d80a7a to 6808f9c Compare July 26, 2023 14:07
@Peter-Lavigne
Copy link
Author

I rebased, regenerated the version bump commit, and force-pushed to keep commit history clean.

@Peter-Lavigne
Copy link
Author

Per this comment, I'm closing this in favor of a new PR to possibly resolve CI storage issues.

@Peter-Lavigne
Copy link
Author

New PR: #446

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants