Conversation
📝 WalkthroughWalkthroughAdds Docker containerization files and a new remote-backed plugin store with periodic refresh: a Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile (1)
33-34: Consider adding--chownfor explicit file ownership.Files copied after
USER marketplaceare still root-owned (COPY always runs as root). While this works since the files only need read/execute permissions, explicit ownership improves clarity and avoids potential confusion.♻️ Optional: Add explicit ownership
-COPY --from=builder /marketplace /app/marketplace -COPY plugins.json /app/plugins.json +COPY --from=builder --chown=marketplace:marketplace /marketplace /app/marketplace +COPY --chown=marketplace:marketplace plugins.json /app/plugins.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 33 - 34, The COPY instructions that run after USER marketplace still create root-owned files; update the two COPY commands ("COPY --from=builder /marketplace /app/marketplace" and "COPY plugins.json /app/plugins.json") to include explicit ownership (e.g., --chown=marketplace:marketplace) so the copied files are owned by the marketplace user and avoid root-owned artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.dockerignore:
- Line 2: The .dockerignore excludes .git/ which prevents the Docker build stage
(the Dockerfile commands using git describe and git rev-parse) from retrieving
real commit/version data, causing the /health endpoint to report fallback
values; fix by removing the `.git/` entry from .dockerignore so the .git
directory is included in the build context, or alternatively modify the
Dockerfile to accept explicit build args (e.g., VERSION, COMMIT) and update the
build invocation to pass those values so git describe/rev-parse are not
required.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 33-34: The COPY instructions that run after USER marketplace still
create root-owned files; update the two COPY commands ("COPY --from=builder
/marketplace /app/marketplace" and "COPY plugins.json /app/plugins.json") to
include explicit ownership (e.g., --chown=marketplace:marketplace) so the copied
files are owned by the marketplace user and avoid root-owned artifacts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f887a6c3-91b2-4397-8c2d-196ef10a9e02
📒 Files selected for processing (3)
.dockerignoreDockerfilecustom-plugins.json
| @@ -0,0 +1,6 @@ | |||
| dist/ | |||
| .git/ | |||
There was a problem hiding this comment.
Excluding .git/ will cause build metadata to always use fallback values.
Since the Dockerfile's build stage uses git describe and git rev-parse (lines 13-15) to populate version info, excluding .git/ from the build context means these commands will always fail and fall back to "dev" and "unknown". The /health endpoint will report these fallback values instead of actual version information.
If build metadata is desired, remove .git/ from this file. If reproducible builds with static metadata are preferred, consider documenting this behaviour or passing version info via build args instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.dockerignore at line 2, The .dockerignore excludes .git/ which prevents the
Docker build stage (the Dockerfile commands using git describe and git
rev-parse) from retrieving real commit/version data, causing the /health
endpoint to report fallback values; fix by removing the `.git/` entry from
.dockerignore so the .git directory is included in the build context, or
alternatively modify the Dockerfile to accept explicit build args (e.g.,
VERSION, COMMIT) and update the build invocation to pass those values so git
describe/rev-parse are not required.
Add --database-url flag to fetch plugins.json from a remote HTTP URL (e.g. raw GitLab/GitHub file) with periodic background refresh via --database-refresh-interval (default 5m). This enables updating the marketplace plugin list without restarting the container. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/marketplace/server.go`:
- Around line 53-67: Parse the database URL using net/url (e.g., call url.Parse
on the value retrieved via command.Flags().GetString("database-url")), validate
that parsing succeeded and that the scheme is exactly "http" or "https"
(case-insensitive), and return an error if not valid; only when valid call
store.NewRemote with the parsed URL, and ensure any logged URL
(logger.WithFields) uses a redacted form (strip User info and sensitive query
params) rather than the raw databaseURL before logging; keep remoteStore.Stop()
defer and apiStore assignment unchanged.
- Around line 134-135: The signal handler currently only listens for
os.Interrupt (SIGINT), so modify the signal registration to also listen for
syscall.SIGTERM (e.g., signal.Notify(stopCh, os.Interrupt, syscall.SIGTERM)) in
the same scope where the shutdown is triggered; keep the existing shutdown flow
that calls srv.Shutdown(ctx) and logs with logger.WithField("err",
err).Error("Failed to shutdown") so that docker stop (SIGTERM) triggers the same
graceful shutdown path. Ensure you import "syscall" if not already imported.
In `@internal/store/remote.go`:
- Around line 47-65: The refresh function uses http.Get and io.ReadAll which
have no timeout or size cap; change RemoteStore.refresh to create a request with
a cancellable context with a timeout (use either r.ctx if Remote has a
background/context used for Stop or create a child context with
time.AfterTimeout) and use http.Client/Do or http.NewRequestWithContext instead
of http.Get so the call can be cancelled; when reading the response, wrap
resp.Body with an io.LimitReader (e.g., maxResponseSize) and decode via
model.PluginsFromReader from that reader rather than io.ReadAll to prevent
unbounded memory usage; ensure resp.Body is closed and context errors are
propagated/translated to the returned error so Stop can interrupt in-flight
fetches.
- Around line 30-44: NewRemote should validate the refreshInterval before
starting refreshLoop: check that the passed refreshInterval is > 0 and return a
descriptive error if not, so NewRemote rejects non-positive durations instead of
allowing refreshLoop (which uses time.NewTicker) to panic; update NewRemote to
perform this check (before assigning to RemoteStore or calling r.refresh) and
reference RemoteStore.interval, NewRemote, refreshLoop and refresh when making
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b4d9fb6-d25c-4b44-bff4-cb1f25632590
📒 Files selected for processing (2)
cmd/marketplace/server.gointernal/store/remote.go
| databaseURL, _ := command.Flags().GetString("database-url") | ||
| if databaseURL != "" && strings.HasPrefix(databaseURL, "http") { | ||
| refreshInterval, _ := command.Flags().GetDuration("database-refresh-interval") | ||
| logger.WithFields(logrus.Fields{ | ||
| "url": databaseURL, | ||
| "interval": refreshInterval, | ||
| }).Info("Using remote database URL") | ||
|
|
||
| remoteStore, remoteErr := store.NewRemote(databaseURL, refreshInterval, logger) | ||
| if remoteErr != nil { | ||
| return errors.Wrap(remoteErr, "failed to initialize remote store") | ||
| } | ||
| defer remoteStore.Stop() | ||
| apiStore = remoteStore | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the code at the specified location in cmd/marketplace/server.go
cat -n cmd/marketplace/server.go | sed -n '50,70p'Repository: mattermost/mattermost-marketplace
Length of output: 976
🏁 Script executed:
# Check the internal/store/remote.go around line 48 for logging concerns
cat -n internal/store/remote.go | sed -n '45,55p'Repository: mattermost/mattermost-marketplace
Length of output: 470
🏁 Script executed:
# Search for URL validation and logging patterns in the store package
rg -A 3 -B 3 "HasPrefix.*http" cmd/ internal/Repository: mattermost/mattermost-marketplace
Length of output: 562
🏁 Script executed:
# Check how NewRemote function handles URL parsing
rg -A 10 "func NewRemote" internal/store/Repository: mattermost/mattermost-marketplace
Length of output: 694
Parse and validate --database-url with proper scheme checking instead of prefix matching.
The current strings.HasPrefix(databaseURL, "http") check is case-sensitive and inadequate:
HTTPS://...orHttp://...incorrectly fall through to local-file mode- Invalid schemes like
httpx://...are incorrectly accepted as remote - No validation of actual URL structure
Additionally, the raw URL is logged at both info level (line 57) and debug level in internal/store/remote.go line 48, creating a security risk for embedded credentials or signed query parameters. Parse the URL once using Go's net/url package, require only http or https schemes (case-insensitive), fail fast on invalid input, and log a redacted form instead of the raw URL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/marketplace/server.go` around lines 53 - 67, Parse the database URL using
net/url (e.g., call url.Parse on the value retrieved via
command.Flags().GetString("database-url")), validate that parsing succeeded and
that the scheme is exactly "http" or "https" (case-insensitive), and return an
error if not valid; only when valid call store.NewRemote with the parsed URL,
and ensure any logged URL (logger.WithFields) uses a redacted form (strip User
info and sensitive query params) rather than the raw databaseURL before logging;
keep remoteStore.Stop() defer and apiStore assignment unchanged.
| if err := srv.Shutdown(ctx); err != nil { | ||
| logger.WithField("err", err).Error("Failed to shutdown") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | head -20Repository: mattermost/mattermost-marketplace
Length of output: 520
🏁 Script executed:
fd -t f "server.go" cmd/Repository: mattermost/mattermost-marketplace
Length of output: 102
🏁 Script executed:
wc -l cmd/marketplace/server.goRepository: mattermost/mattermost-marketplace
Length of output: 106
🏁 Script executed:
sed -n '1,160p' cmd/marketplace/server.go | cat -nRepository: mattermost/mattermost-marketplace
Length of output: 5329
Handle SIGTERM for Docker container graceful shutdown.
The signal handler on line 126 currently only registers os.Interrupt (SIGINT), which means graceful shutdown will not occur when Docker sends SIGTERM via docker stop. This needs to be added for proper container lifecycle support.
Suggested fix
import (
"context"
"log"
"net/http"
"os"
"os/signal"
"strings"
+ "syscall"
"time"
@@
- signal.Notify(c, os.Interrupt)
+ signal.Notify(c, os.Interrupt, syscall.SIGTERM)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/marketplace/server.go` around lines 134 - 135, The signal handler
currently only listens for os.Interrupt (SIGINT), so modify the signal
registration to also listen for syscall.SIGTERM (e.g., signal.Notify(stopCh,
os.Interrupt, syscall.SIGTERM)) in the same scope where the shutdown is
triggered; keep the existing shutdown flow that calls srv.Shutdown(ctx) and logs
with logger.WithField("err", err).Error("Failed to shutdown") so that docker
stop (SIGTERM) triggers the same graceful shutdown path. Ensure you import
"syscall" if not already imported.
| func NewRemote(url string, refreshInterval time.Duration, logger logrus.FieldLogger) (*RemoteStore, error) { | ||
| r := &RemoteStore{ | ||
| url: url, | ||
| interval: refreshInterval, | ||
| logger: logger, | ||
| stopCh: make(chan struct{}), | ||
| } | ||
|
|
||
| if err := r.refresh(); err != nil { | ||
| return nil, errors.Wrap(err, "failed initial fetch from remote URL") | ||
| } | ||
|
|
||
| go r.refreshLoop() | ||
|
|
||
| return r, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file structure
find . -name "remote.go" -type f | head -20Repository: mattermost/mattermost-marketplace
Length of output: 103
🏁 Script executed:
# Look at the remote.go file to see the full implementation
cat -n internal/store/remote.goRepository: mattermost/mattermost-marketplace
Length of output: 3366
🏁 Script executed:
# Search for refreshLoop implementation
rg "func.*refreshLoop" -A 10Repository: mattermost/mattermost-marketplace
Length of output: 628
🏁 Script executed:
# Check for interval validation in the codebase
rg "refreshInterval|refresh-interval|database-refresh-interval" -iRepository: mattermost/mattermost-marketplace
Length of output: 702
🏁 Script executed:
# Check how the interval parameter is used
rg "NewTicker.*interval" -B 2 -A 2Repository: mattermost/mattermost-marketplace
Length of output: 300
🌐 Web query:
Go time.NewTicker non-positive duration panic documentation
💡 Result:
time.NewTicker(d) requires d > 0. The Go time package docs state: “The duration d must be greater than zero; if not, NewTicker will panic.” [1]
In the standard library implementation, the panic happens when d <= 0, with the panic message: panic("non-positive interval for NewTicker"). [2]
Sources:
[1] Go time package docs (NewTicker) [1]
[2] Go standard library source (src/time/tick.go) [2]
Add validation to reject non-positive refresh intervals in NewRemote.
time.NewTicker panics if the duration is zero or negative, which means a misconfigured --database-refresh-interval flag will crash the process inside the refreshLoop() goroutine after the initial fetch succeeds. This should be caught and returned as a validation error during initialization instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/store/remote.go` around lines 30 - 44, NewRemote should validate the
refreshInterval before starting refreshLoop: check that the passed
refreshInterval is > 0 and return a descriptive error if not, so NewRemote
rejects non-positive durations instead of allowing refreshLoop (which uses
time.NewTicker) to panic; update NewRemote to perform this check (before
assigning to RemoteStore or calling r.refresh) and reference
RemoteStore.interval, NewRemote, refreshLoop and refresh when making the change.
| func (r *RemoteStore) refresh() error { | ||
| r.logger.WithField("url", r.url).Debug("Fetching plugins from remote URL") | ||
|
|
||
| resp, err := http.Get(r.url) // #nosec G107 -- URL is operator-configured, not user input | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to fetch remote plugins") | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("remote URL returned status %d", resp.StatusCode) | ||
| } | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to read response body") | ||
| } | ||
|
|
||
| plugins, err := model.PluginsFromReader(bytes.NewReader(body)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the file structure and find the complete refresh() method
wc -l internal/store/remote.goRepository: mattermost/mattermost-marketplace
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Read the complete file to understand context and any timeout/context usage
cat -n internal/store/remote.goRepository: mattermost/mattermost-marketplace
Length of output: 3366
🏁 Script executed:
#!/bin/bash
# Search for any context or timeout related code in the store package
rg -n "context|timeout|Context|Timeout" internal/store/ -iRepository: mattermost/mattermost-marketplace
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Look for the Stop() method and how it interacts with refresh()
rg -n "func.*Stop|Stop()" internal/store/ -A 5Repository: mattermost/mattermost-marketplace
Length of output: 789
Add timeout and size limits to remote fetch operations.
http.Get uses the default client with no timeout, and io.ReadAll buffers the entire response body without a size cap. A stalled or oversized response can hang start-up indefinitely (blocking NewRemote), and during periodic refresh, Stop() cannot interrupt an in-flight fetch since it only signals the refresh loop's select statement—a blocked http.Get or io.ReadAll call won't be cancelled. Use a context with timeout and decode through a bounded reader instead of buffering the full payload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/store/remote.go` around lines 47 - 65, The refresh function uses
http.Get and io.ReadAll which have no timeout or size cap; change
RemoteStore.refresh to create a request with a cancellable context with a
timeout (use either r.ctx if Remote has a background/context used for Stop or
create a child context with time.AfterTimeout) and use http.Client/Do or
http.NewRequestWithContext instead of http.Get so the call can be cancelled;
when reading the response, wrap resp.Body with an io.LimitReader (e.g.,
maxResponseSize) and decode via model.PluginsFromReader from that reader rather
than io.ReadAll to prevent unbounded memory usage; ensure resp.Body is closed
and context errors are propagated/translated to the returned error so Stop can
interrupt in-flight fetches.
1 participant
Add Dockerfile and supporting files to enable self-hosted marketplace deployments. This allows users to build a container image of the marketplace server and mount a custom plugins.json to serve their own curated set of plugins.
Also adds a --database-url flag that fetches plugins.json from a remote HTTP URL (e.g. a raw GitLab/GitHub file) with periodic background refresh, enabling live plugin list updates without restarting the container.
Files added:
Files modified:
Usage:
Build the image
docker build -t mattermost-marketplace .
Run with built-in plugins.json
docker run -d -p 8085:8085 mattermost-marketplace
Run with a custom local plugins.json
docker run -d -p 8085:8085 -v $(pwd)/custom-plugins.json:/app/plugins.json:ro mattermost-marketplace
Run with a remote URL (auto-refreshes every 5 minutes)
docker run -d -p 8085:8085 mattermost-marketplace server \
--database-url "https://gitlab.com/mygroup/myrepo/-/raw/main/plugins.json" \
--database-refresh-interval 5m \
--listen :8085
Ticket Link
N/A