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

Improve Quality Summary #99

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pauljmartinez
Copy link

This PR attempts to resolve #10; however, I only just started using Web-Check and this may not be the preferred approach. That said, I’m happy to incorporate any feedback to make this PR more useful. :)

One unresolved issue is related to api/threats.js which appears to indicate that GOOGLE_CLOUD_API_KEY may be intended to work for both Page Speed Insights (Lighthouse) and Google Safe Browsing. Any clarification here would be greatly appreciated.

The gist of this update is to rename the API key environment variable and to provide a more direct URL to assist users with adding support for the Quality Summary section of the report.

Additionally, this may introduce a breaking change for current users as they would need to modify their environment variables. I’m not sure how to best handle this type of situation.

This PR attempts to resolve Lissy93#10; however, I only just started using Web-Check and this may not be the preferred approach. That said, I’m happy to incorporate any feedback to make this PR more useful. :)

One unresolved issue is related to `api/threats.js` which appears to indicate that GOOGLE_CLOUD_API_KEY may be intended to work for both Page Speed Insights (Lighthouse) and Google Safe Browsing. Any clarification here would be greatly appreciated.

The gist of this update is to rename the API key environment variable and to provide a more direct URL to assist users with adding support for the Quality Summary section of the report.

Additionally, this may introduce a breaking change for current users as they would need to modify their environment variables. I’m not sure how to best handle this type of situation.
Copy link

netlify bot commented Mar 1, 2024

Deploy Preview for web-check ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit d738aaf
🔍 Latest deploy log https://app.netlify.com/sites/web-check/deploys/65e230fdefa88c0008240efd
😎 Deploy Preview https://deploy-preview-99--web-check.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Owner

@Lissy93 Lissy93 left a comment

Choose a reason for hiding this comment

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

Is it necessary to change the env var name?
I'm just thinking... this is going to be a breaking change if we do, for everyone whose already running the :latest container. And like you mentioned, it's the same key used for other checks relying on services provided by Google Cloud services

@pauljmartinez
Copy link
Author

Totally not necessary and I'd be happy to resubmit without making that change!

I'm unable to tell if that key I generated at https://developers.google.com/speed/docs/insights/v5/get-started is able to be used by the Google Safe Browsing service though.

@Lissy93
Copy link
Owner

Lissy93 commented Mar 3, 2024

I'm unable to tell if that key I generated at https://developers.google.com/speed/docs/insights/v5/get-started is able to be used by the Google Safe Browsing service though.

It's the same key, but you need to enable the safe browsing API in the Google dev dashboard

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.

feature request show IPv4 and IPv6 under Server Info
3 participants