-
Notifications
You must be signed in to change notification settings - Fork 1
Superset: Usage guides #282
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
Conversation
WalkthroughAdds two new Superset guides (Usage and Sandbox) and updates the Superset index: inserts Usage grid card and revised Blog cards, replaces inline links with a Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as Superset UI
participant API as Superset Backend (API)
participant D as CrateDB SQLAlchemy Dialect
participant DB as CrateDB
U->>UI: Open Superset
UI->>API: Authenticate (login)
note over UI,API: Obtain JWT + CSRF tokens
U->>UI: Configure DB connection
UI->>API: Save connection (CrateDB URI)
API->>D: Create engine / compile SQL
D->>DB: Query over HTTP
DB-->>D: Return rows
D-->>API: Deliver rows/metadata
API-->>UI: Present data
UI-->>U: Render charts / tables
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/integrate/superset/usage.md (1)
136-136
: Document assumption about database ID.The command assumes
database=1
, which implies this is the first database connection created. If users have already created other databases in their Superset instance, this ID might be incorrect. Consider adding a note that users should verify the database ID from the previous API response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/integrate/superset/index.md
(3 hunks)docs/integrate/superset/sandbox.md
(1 hunks)docs/integrate/superset/usage.md
(1 hunks)
🔇 Additional comments (5)
docs/integrate/superset/index.md (2)
169-175
: LGTM! Toctree structure is well-organized.The hidden toctree properly references the new usage and sandbox documentation files, providing clean navigation without cluttering the page content.
92-92
: Blog post URLs accessible. Verified both CrateDB blog links return HTTP 200 and are accessible to users.docs/integrate/superset/usage.md (2)
87-87
: S3 sample data URL returns HTTP 200 OK — no further action required.
7-7
: No action required: example repository link is valid. Theapplication/apache-superset
directory exists in the crate/cratedb-examples repository, so the referenced URL is correct.docs/integrate/superset/sandbox.md (1)
34-34
: External link verified: The community article URL returns HTTP 200 and is accessible.
docs/integrate/superset/sandbox.md
Outdated
export NODEJS_VERSION=16.15.1 | ||
export NPM_VERSION=7 | ||
source /dev/stdin <<<"$(curl -s https://raw.githubusercontent.com/cicerops/supernode/main/supernode)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify Node.js and npm versions are still supported.
Node.js 16.15.1 reached end-of-life in September 2023, and npm 7 is several major versions behind. Consider updating to a currently supported LTS version of Node.js (18.x or 20.x) to ensure security updates and compatibility.
🌐 Web query:
What is the current LTS version of Node.js and when did Node.js 16 reach end-of-life?
💡 Result:
- Current LTS: Node.js 22.x (Active LTS). [1]
- Node.js 16 reached end-of-life on September 11, 2023. [2]
Sources: Node.js Releases page and Node.js announcement about v16 EOL. [1][2]
Update Node.js and npm versions to supported LTS
Node.js 16.15.1 reached end-of-life on September 11, 2023; the current Active LTS is Node.js 22.x. npm 7 is several major versions behind—update both to the versions bundled with Node.js 22.x to receive ongoing security fixes and compatibility updates.
🤖 Prompt for AI Agents
In docs/integrate/superset/sandbox.md around lines 52 to 54, the file pins
Node.js 16.15.1 and npm 7 which are EOL/unsupported; update the environment
variables to use the current Active LTS (Node.js 22.x) and the npm version
bundled with Node 22.x (replace NODEJS_VERSION and NPM_VERSION accordingly), and
verify the sourced supernode script works with those versions.
# Setup Node.js 16 with NPM 7. | ||
export NODEJS_VERSION=16.15.1 | ||
export NPM_VERSION=7 | ||
source /dev/stdin <<<"$(curl -s https://raw.githubusercontent.com/cicerops/supernode/main/supernode)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security consideration: piping curl output to shell.
Piping curl output directly to a shell interpreter can be risky if the remote source is compromised. While this pattern is used by many legitimate tools, consider adding a note about reviewing the script content first or pinning to a specific commit hash instead of using main
branch.
... pulling more content from the community forum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/integrate/superset/usage.md (2)
21-21
: Consider pinning package versions for reproducibility.While the current approach works, consider pinning versions (e.g.,
apache-superset==4.1.0
) to ensure reproducible installations and avoid potential breaking changes when new versions are released.
111-147
: HTTP API examples are clear and functional.The HTTPie commands demonstrate the authentication flow and API usage effectively. The note about the database ID assumption (lines 143-147) is a helpful caveat for users.
One minor consideration: Using
--session=superset
persists credentials to disk in HTTPie's session files. For production environments, consider mentioning this security implication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/integrate/superset/index.md
(3 hunks)docs/integrate/superset/sandbox.md
(1 hunks)docs/integrate/superset/usage.md
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/integrate/superset/sandbox.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/integrate/superset/index.md
92-92: Bare URL used
(MD034, no-bare-urls)
98-98: Bare URL used
(MD034, no-bare-urls)
docs/integrate/superset/usage.md
114-114: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
130-130: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build docs
🔇 Additional comments (10)
docs/integrate/superset/usage.md (6)
1-8
: LGTM!The introduction clearly sets expectations and provides helpful context by referencing the source recipe used in CI systems.
39-53
: LGTM!The Docker command for CrateDB uses appropriate flags for a development setup, and the Superset server configuration is suitable for local testing.
56-95
: LGTM!The data loading workflow is well-structured. Using
--network=host
is appropriate for connecting to CrateDB running locally, and the SQL schema matches the NYC taxi dataset structure.
102-108
: LGTM!The Web UI section provides clear navigation instructions and appropriately links to official Superset documentation for detailed procedures.
114-114
: Static analysis hint can be safely ignored.The linter flags these bold labels as improper heading usage, but in this context they serve as visual markers for code block groups rather than document structure headings. This is an acceptable documentation pattern.
Also applies to: 130-130
149-152
: LGTM!The cleanup instructions are comprehensive and cover all components introduced in the guide.
docs/integrate/superset/index.md (4)
77-84
: LGTM!The new grid card properly references the usage guide with the correct Sphinx
:ref:
link type and matches the label defined inusage.md
.
86-103
: LGTM!The Blog section is well-structured. The updated title "Introduction to time-series visualization" provides better context than the previous version.
The static analysis warnings about bare URLs (lines 92, 98) are false positives—these URLs are properly used as arguments to the
:link:
directive in MyST syntax.
160-160
: LGTM!The reference to
superset-sandbox
uses the correct Sphinx syntax and aligns with the new sandbox documentation mentioned in the PR objectives.
169-174
: LGTM!The toctree is properly structured with appropriate options (
:hidden:
and:maxdepth: 1
) and correctly references the new documentation files.
About
Continue adding integration guides from the community forum.
Preview
References