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

Skip Browserless checks in CE #4601

Closed
wants to merge 1 commit into from

Conversation

ruslandoga
Copy link
Contributor

Changes

Attempt at skipping Browserless checks in CE: #4459 (comment)

Tests

  • Automated tests have been added

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@ruslandoga ruslandoga changed the title skip browserless checks in ce Skip browserless checks in CE Sep 20, 2024
@ruslandoga ruslandoga changed the title Skip browserless checks in CE Skip Browserless checks in CE Sep 20, 2024
Checks.FetchBody,
Checks.CSP,
Checks.ScanBody,
Checks.Snippet,
Checks.SnippetCacheBust,
Checks.SnippetCacheBust
Copy link
Member

Choose a reason for hiding this comment

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

without Checks.Installation, verification won't ever succeed so in that sense this is equivalent to querying a non-existing browserless endpoint.

Copy link
Contributor Author

@ruslandoga ruslandoga Sep 23, 2024

Choose a reason for hiding this comment

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

Is there a way around it? I assumed the verification succeeds after all (or a subset of?) defined checks pass.

Copy link
Member

Choose a reason for hiding this comment

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

sort of - but plausible_installed? is crucial (and even then - it's not binary, if you skim through the diagnostics module), which comes from the browserless check. Take a look at Plausible.Verification.Diagnostics it's where the decision about user-facing result is made.

Feels also pointless to show verification success to the user if you don't run verification.

We could consider disabling it altogether again, by removing "live "/:website/verification", Verification, :verification, as: :site" route in CE and following up on the consequences. PlausibleWeb.Flows comes to mind as the next thing to deal with...

@ruslandoga ruslandoga closed this Sep 23, 2024
@ruslandoga ruslandoga deleted the skip-browserless-checks-in-ce branch September 23, 2024 05:46
@ruslandoga ruslandoga mentioned this pull request Sep 23, 2024
4 tasks
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.

2 participants