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

docs: update docker container and celery worker chrome installation #29315

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Malcolm
Copy link

@Malcolm Malcolm commented Jun 20, 2024

SUMMARY

The existing documentation has the user download the latest release of Chrome and use ChromeDriver v102. This combination doesn't function, providing the user with an error saying the Chrome version is unsupported from ChromeDriver during alert/report generation.

This update pulls both the version of Chrome and ChromeDriver from the Chrome Labs repository which is centrally maintained and allows for both the Chrome and ChromeDriver version to stay in lock step.

Additionally the Superset base image version and Chrome versions can be set as build-args during image creation.

TESTING INSTRUCTIONS

  1. Build the container as per docs/docs/configuration/alerts-reports.mdx with a customised tag
  2. Update x-superset-image in docker-compose.yml to point to your customised tag
  3. Start the stack, configure SMTP, create then fire a report

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot bot added alert-reports Namespace | Anything related to the Alert & Reports feature doc Namespace | Anything related to documentation labels Jun 20, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@john-bodley john-bodley requested a review from sfirke June 20, 2024 18:10
@john-bodley john-bodley added the review:checkpoint Last PR reviewed during the daily review standup label Jun 20, 2024
@michael-s-molina michael-s-molina removed the review:checkpoint Last PR reviewed during the daily review standup label Jun 21, 2024
@sfirke
Copy link
Member

sfirke commented Aug 16, 2024

@Malcolm Thanks for this PR and sorry it took so long for you to get a review! It looks great to me but I haven't set up Chrome for my reporting and this isn't something I know much about. I've put this to a few people for review and hopefully someone can take a look.

And if no one is able to weigh in, I'd say let's err on the side of merging this - I'll put a calendar reminder for myself to circle back here.

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

Can we switch to a solution that is deterministic?

docs/docs/configuration/alerts-reports.mdx Show resolved Hide resolved
@mistercrunch mistercrunch dismissed their stale review August 16, 2024 21:13

Sorry I didn't see that we hard-code the CHROME_VERSION

@mistercrunch
Copy link
Member

Disregard my previous review/comment, is there not an easier/cleaner way to install this!? I didn't like the way we did it before, and this seems like a good iteration based on this approach, but would there be a better way?

@mistercrunch
Copy link
Member

Hey, looking at playwright, it seems we may be able to streamline things quite a bit by consistently using it to install headless browsers, both chromium and firefox. Wondering if we should target that instead. Also wondering if we could make sure we install versions while using playwright, something like playwright install --with-deps [email protected] [email protected] seems like it may work and simplify both our dockerfile and documentation

@hoalongnatsu
Copy link
Contributor

Maybe we don't need to run:

apt-get install -y libglib2.0-0 libnss3 libxcb1 libatk1.0-0 libatk-bridge2.0-0 \
        libcups2 libdrm2 libxkbcommon0 libxcomposite1 libxdamage1 \
        libxfixes3 libxrandr2 libgbm1 libpango-1.0-0 libcairo2 libasound2

This command will install these packages:

apt-get install -y --no-install-recommends ./google-chrome-*.deb

Additionally, we can obtain a version of Chrome using a command, so we don't need to fix one:

export CHROMEDRIVER_VERSION=$(curl --silent https://googlechromelabs.github.io/chrome-for-testing/LATEST_RELEASE_116)

@mistercrunch
Copy link
Member

Can we look into using the playwright approach instead? Seems it would simplify all this greatly - both for Firefox and Chromium

@rusackas rusackas requested a review from kgabryje August 19, 2024 16:06
@sfirke sfirke removed their request for review August 27, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alert-reports Namespace | Anything related to the Alert & Reports feature doc Namespace | Anything related to documentation size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants