-
Notifications
You must be signed in to change notification settings - Fork 183
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
create Google app creds file from env var as needed #4366
Conversation
d3f47e8
to
4f58897
Compare
4f58897
to
c965fe5
Compare
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.
Thanks @groovecoder, this looks a little safer. Please consider what should happen if GOOGLE_APPLICATION_CREDENTIALS
is a directory - raise an exception, or skip loading the profiler?
privaterelay/apps.py
Outdated
gcp_key_json_path = Path(settings.GOOGLE_APPLICATION_CREDENTIALS) | ||
if gcp_key_json_path.exists() and gcp_key_json_path.is_file(): | ||
if not gcp_key_json_path.exists() or not gcp_key_json_path.is_file(): |
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 you set GOOGLE_APPLICATION_CREDENTIALS
to "privaterelay"
, this fails with IsADirectoryError: [Errno 21] Is a directory: 'privaterelay'
. I think the line should be:
if not gcp_key_json_path.exists() or not gcp_key_json_path.is_file(): | |
if not gcp_key_json_path.exists() and gcp_key_json_path.is_file(): |
At the same time, you'll get a IsADirectoryError
in a couple of lines when you try to read it. So, maybe drop the is_file()
check and let Relay crash on startup.
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.
I removed the is_file
check and expanded the try/except
block around the code for the file so that the Relay app can still start up and run even if google cloud profiler can't.
c965fe5
to
92c341f
Compare
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.
Thanks, this worked with my tests (folder, empty json). One suggestion you could take or leave.
privaterelay/apps.py
Outdated
except: | ||
print( | ||
f"exception while starting google cloud profiler with key file: {gcp_key_json_path}" | ||
) |
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.
Suggestion: print more of the error:
except: | |
print( | |
f"exception while starting google cloud profiler with key file: {gcp_key_json_path}" | |
) | |
except Exception as exc: | |
print( | |
f"exception {repr(exc)}" | |
" while starting google cloud profiler" | |
f" with key file: {gcp_key_json_path}" | |
) |
92c341f
to
fbc2553
Compare
This PR fixes MPP-3467.
It changes the profiling code to automatically create the necessary GCP key JSON file from the value of the
GOOGLE_CLOUD_PROFILER_CREDENTIALS_B64
environment variable. This should work identically across local, dev, stage, and prod environments.How to test:
runserver
you should see output lines:HTTPAdapter.get_connection url: http://metadata/computeMetadata/v1/instance/zone
which indicates that the profiler code is workingl10n changes have been submitted to the l10n repository, if any.All UI revisions follow the coding standards, and use Protocol tokens where applicable (see/frontend/src/styles/tokens.scss
).