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.

Sign up for GitHub

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

Jump to bottom

Disable buttons/links to share data with other websites depending on environment variables #1528

Merged
merged 5 commits into from
Oct 4, 2024
Merged

Disable buttons/links to share data with other websites depending on environment variables #1528

merged 5 commits into from
Oct 4, 2024

Conversation

bluprince13
Copy link
Contributor

@bluprince13 bluprince13 commented Sep 6, 2024 •
edited
Loading

📑 Summary

One of the reasons users might self-host is because they want to prevent data being leaked out of their company/org. However, there are buttons on the UI that allow users to share their diagram with other website. For example, with mermaid.ink or kroki.io or mermaidchart.com. This PR allows disabling those buttons via environment variables.

Resolves #1511

📏 Design Decisions

Describe the way your implementation works or what design decisions you made if applicable:

With no values for the environment variables as shown below, the data sharing buttons/links are not shown:

    MERMAID_DOMAIN=''
    MERMAID_ANALYTICS_URL=''
    MERMAID_RENDERER_URL=''
    MERMAID_KROKI_RENDERER=''
    MERMAID_IS_ENABLED_MERMAID_CHART_LINKS=''

With these environment variable values, the UI looks the same as before.

    MERMAID_DOMAIN='mermaid.live'
    MERMAID_ANALYTICS_URL='https://p.mermaid.live'
    MERMAID_RENDERER_URL='https://mermaid.ink'
    MERMAID_KROKI_RENDERER_URL="https://kroki.io"
    MERMAID_IS_ENABLED_MERMAID_CHART_LINKS="True"

Let me know if you'd prefer the .env file to have values filled in. Since the changes are simple, I haven't added any tests, but can do if you wish. I wanted to get feedback on the approach first.

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 🔖 targeted develop branch

@netlify Netlify
Copy link

netlify bot commented Sep 6, 2024 •
edited
Loading

✅ Deploy Preview for mermaidjs ready!

Name Link
🔨 Latest commit 4415849
🔍 Latest deploy log https://app.netlify.com/sites/mermaidjs/deploys/66ffb23ff34a340008ea6925
😎 Deploy Preview https://deploy-preview-1528--mermaidjs.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.

@bluprince13 bluprince13 force-pushed the feature/1511_ability-to-disable-data-share-buttons branch from 431a63f to 771f658 Compare September 6, 2024 21:53
@bluprince13 bluprince13 marked this pull request as ready for review September 7, 2024 07:39
sidharthv96
sidharthv96 requested changes Sep 16, 2024
View reviewed changes
Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

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

Nice work @bluprince13!
Need some minor changes.

README.md Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
netlify.toml Outdated Show resolved Hide resolved
src/lib/util/env.ts Outdated Show resolved Hide resolved
src/lib/components/Navbar.svelte Outdated Show resolved Hide resolved
src/lib/components/Navbar.svelte Outdated Show resolved Hide resolved
@bluprince13 bluprince13 force-pushed the feature/1511_ability-to-disable-data-share-buttons branch from 771f658 to e6b5415 Compare September 28, 2024 15:07
@bluprince13 bluprince13 force-pushed the feature/1511_ability-to-disable-data-share-buttons branch from e6b5415 to f663318 Compare September 28, 2024 15:12
@bluprince13
Copy link
Contributor Author

Updated to address comments.

sidharthv96
sidharthv96 approved these changes Sep 28, 2024
View reviewed changes
Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

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

There's no need to destructure if we're only using it once.

src/lib/util/promos/promo.ts Outdated Show resolved Hide resolved
src/routes/edit/+page.svelte Outdated Show resolved Hide resolved
@bluprince13
Copy link
Contributor Author

Is there anything more required here or is this mergeable?

@sidharthv96
Copy link
Member

Hey @bluprince13, I started looking into the failing tests, but got distracted. Do you mind fixing those as well? If the failures are valid, it's okay to update the values appropriately.

@bluprince13
Copy link
Contributor Author

@sidharthv96 oh I thought that was just 1 of 3 tries that was failing. I guess not. Will look into it.

@bluprince13
Copy link
Contributor Author

@sidharthv96 isn't this just running the same test suite 3 times? 2/3 times it was successful. So, the tests are just flaky?

See https://github.com/mermaid-js/mermaid-live-editor/blob/develop/.github/workflows/tests.yml#L15

On the the 3rd run, there are failures on:

  1. actions.spec.ts - Should download png and svg
  2. history.spec.ts - Save history -> Which this PR shouldn't impact anyway.

@sidharthv96
Copy link
Member

It's running 3 copies of the same job, but the tests being run in each job is different, as we have the parallel flag set in cypress.

mermaid-live-editor/.github/workflows/tests.yml

Line 48 in 60e2b30

parallel: true

@sidharthv96 sidharthv96 merged commit 8190a94 into mermaid-js:develop Oct 4, 2024
9 of 10 checks passed
@sidharthv96
Copy link
Member

Merging this PR, as the failures are unrelated. Will fix separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sidharthv96 sidharthv96 Awaiting requested review from sidharthv96

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable buttons that share data externally to allow safe self-hosting
2 participants
@bluprince13 @sidharthv96