-
Notifications
You must be signed in to change notification settings - Fork 147
Fix sqlite-web integration for breaking changes in latest container #1061
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
…f env var Co-authored-by: aaronpowell <[email protected]>
|
@geheb there's a NuGet package on our Azure DevOps feed (see the README for that link), do you want to test it and verify if this resolves the issue? |
Minimum allowed line rate is |
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.
Pull request overview
This PR fixes the sqlite-web integration to work with breaking changes in the latest upstream container. The sqlite-web container now requires the database filename as a command-line argument instead of the SQLITE_DATABASE environment variable that was removed.
Key Changes:
- Changed from
.WithEnvironment()to.WithArgs()to pass the database filename as a command-line argument - Updated the test to verify
CommandLineArgsCallbackAnnotationinstead of environment variables
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/CommunityToolkit.Aspire.Hosting.Sqlite/SqliteResourceBuilderExtensions.cs |
Replaced environment variable with command-line argument to pass database filename |
tests/CommunityToolkit.Aspire.Hosting.Sqlite.Tests/AddSqliteTests.cs |
Updated test to verify command-line args instead of environment variables |
Review Summary: The implementation correctly addresses the breaking change in the sqlite-web container. The changes follow established repository patterns for using .WithArgs() (as seen in 20+ other integrations) and the test pattern matches conventions used in other test files (e.g., Java, Flagd, OpenTelemetryCollector tests). The configureContainer callback is invoked after default args are set (line 85), which preserves the documented workaround behavior. No documentation updates are needed since the README doesn't expose these internal implementation details.
Closes #<ISSUE_NUMBER>
The latest sqlite-web container changed from using
SQLITE_DATABASEenvironment variable to requiring the database filename as a command-line argument via itsENTRYPOINT.Changes
.WithEnvironment()to.WithArgs()to pass database filename as command-line argumentCommandLineArgsCallbackAnnotationinstead of environment variablesExample
Now correctly runs the container as
sqlite_web -H 0.0.0.0 -x database.dbinstead of relying on the removedSQLITE_DATABASEenv var.The workaround (
.WithSqliteWeb(c => c.WithArgs("database.db"))) continues to work sinceconfigureContaineris invoked after default args are set.PR Checklist
Other information
Upstream breaking change: coleifer/sqlite-web@49ccbea
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.