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

CARDS-2360: Display startup warning if the GOOGLE_APIKEY environment variable is not set #1605

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

veronikaslc
Copy link
Contributor

@veronikaslc veronikaslc commented Sep 27, 2023

Testing:

  • start without defining the GOOGLE_APIKEY environment variable, check that the warning message is displayed
  • stop cards
  • set the GOOGLE_APIKEY environment variable
  • start cards, check that the warning message is no longer displayed

@veronikaslc veronikaslc added easy to review Very few changes, very small time investment to review Test me! Ready for testing labels Sep 27, 2023
start_cards.sh Outdated Show resolved Hide resolved
sashaandjic

This comment was marked as resolved.

@sashaandjic sashaandjic added tested Passed manual testing, needs code review and removed Test me! Ready for testing labels Sep 27, 2023
@marta- marta- removed the tested Passed manual testing, needs code review label Sep 27, 2023
@sashaandjic sashaandjic added the bug Something isn't working label Sep 27, 2023
@marta- marta- removed the bug Something isn't working label Sep 27, 2023
@marta-

This comment was marked as resolved.

…variable is not set

Change message color from red (error) to yellow (warning)
start_cards.sh Outdated Show resolved Hide resolved
@marta- marta- added the Test me! Ready for testing label Sep 27, 2023
@sashaandjic

This comment was marked as outdated.

@sashaandjic sashaandjic added tested Passed manual testing, needs code review and removed Test me! Ready for testing labels Sep 27, 2023
@marta-
Copy link
Contributor

marta- commented Sep 27, 2023

re-tested

I just made another change wile you are testing, the GOOGLE_APIKEY warning is now yellow.

Minor observations (out of scope):

  • We check for BIOPORTAL_APIKEY only when starting in test mode, perhaps we can do it outside of the test mode as well?

I'm not sure that's necessary. When going to install vocabularies from the Admin, it is clear from the UI that you need an API key to do so and there's a place to add it right there.

  • When an invalid BIOPORTAL_APIKEY is set, the HANCESTRO installation fails. When an invalid GOOGLE_APIKEY is set, nothing happens when starting cards, however the address type fields will not to work.

Not sure if this is where you are going with the second observation, but the color was changed to yellow because nothing "bad" happens immediately at startup.

@marta- marta- added Test me! Ready for testing and removed tested Passed manual testing, needs code review labels Sep 27, 2023
Copy link
Contributor

@sashaandjic sashaandjic left a comment

Choose a reason for hiding this comment

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

retested - yellow color for missing GOOGLE_APIKEY
image

in test mode:
image

@sashaandjic sashaandjic added tested Passed manual testing, needs code review and removed Test me! Ready for testing labels Sep 27, 2023
environment.md Outdated Show resolved Hide resolved
@IntegralProgrammer
Copy link
Member

Minor observations (out of scope):

  • We check for BIOPORTAL_APIKEY only when starting in test mode, perhaps we can do it outside of the test mode as well?

I'm not sure that's necessary. When going to install vocabularies from the Admin, it is clear from the UI that you need an API key to do so and there's a place to add it right there.

The reason why the check for the BIOPORTAL_APIKEY is only done when the test run mode is enabled is because the test run mode installs the HANCESTRO vocabulary as such is a requirement for certain test forms (ie. ones that have cards:VocabularyAnswer typed questions). If HANCESTRO fails to install when the test run mode is enabled, then there definitely will be forms that don't work properly, There are however many use cases where vocabularies don't need to be installed from BioPortal (such as HERACLES, DATA-PRO, and Your Experience) and thus displaying a missing BIOPORTAL_APIKEY warning here wouldn't really be that meaningful of a warning.

  • When an invalid BIOPORTAL_APIKEY is set, the HANCESTRO installation fails. When an invalid GOOGLE_APIKEY is set, nothing happens when starting cards, however the address type fields will not to work.

Not sure if this is where you are going with the second observation, but the color was changed to yellow because nothing "bad" happens immediately at startup.

It is true that address type fields will not work if the GOOGLE_APIKEY is missing or is invalid but this is only really a problem if the CARDS project uses address type fields. As we are soon moving to using separate repositories for the various CARDS projects that we support, I think what I'd like to do would be to add a validate_environment.sh script to the project-specific Docker images (eg. ghcr.io/data-team-uhn/cards4sparc) which upon execution would validate that the required environment variables are present and are set to valid values. This way, when a project-specific Docker image is started, it will immediately fail (or at the very least generate warnings) as required environment variables are missing/invalid.

So perhaps the best plan for now would be like this:

  • Since none of our other projects support address typed questions, let's have start_cards.sh check for a missing GOOGLE_APIKEY only if no -P or --project has been specified.
  • We'll create a validate_environment.sh script in the https://github.com/data-team-uhn/cards4sparc repository that will check if GOOGLE_APIKEY is set (and we can even validate it against Google's API endpoints).
  • When starting Cards4SPARC without Docker, we can call validate_environment.sh before start_cards.sh which will throw an error is GOOGLE_APIKEY is missing/invalid.
  • The validate_environment.sh script would also be included in the generated ghcr.io/data-team-uhn/cards4sparc Docker image so that Cards4SPARC would fail to start if GOOGLE_APIKEY is missing/invalid.

TL;DR

So just keeping within the scope of this task, I recommend checking for GOOGLE_APIKEY only if no -P or --project has been specified.

Copy link
Member

@IntegralProgrammer IntegralProgrammer left a comment

Choose a reason for hiding this comment

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

Please see #1605 (comment)

@veronikaslc
Copy link
Contributor Author

veronikaslc commented Sep 29, 2023

It is true that address type fields will not work if the GOOGLE_APIKEY is missing or is invalid

Not true, just the address suggestions from Google Places service won't work. From user perspective nothing will be broken, the field becomes just a plain text input.

@IntegralProgrammer
Copy link
Member

It is true that address type fields will not work if the GOOGLE_APIKEY is missing or is invalid

Not true, just the address suggestions from Google Places service won't work. From user perspective nothing will be broken, the field becomes jst a plain text input

Okay, that's good. So in that case, I'd still recommend only checking if GOOGLE_APIKEY is present if no -P / --project is specified but I'd simply print a warning with a bit more detail (eg. "Place suggestions won't work" / "Addresses will only work as plain text input") and for the specific projects we can define, through validate_environment.sh if the missing/invalid GOOGLE_APIKEY constitutes an error, warning, or should simply be ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy to review Very few changes, very small time investment to review low risk tested Passed manual testing, needs code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants