Add shovel and federation management commands to lavinmqctl #1277

Copilot · 2025-09-15T12:36:14Z

The implementation now correctly uses the existing parameters infrastructure for both shovels and federation, avoiding unnecessary new endpoints in shovels.cr. Shovels are managed via /api/parameters/shovel/ just like federation upstreams use /api/parameters/federation-upstream/.

Default Values

Default values are now derived from the actual constants in the codebase:

Shovel defaults:

src-prefetch-count: LavinMQ::Shovel::DEFAULT_PREFETCH (1000)
reconnect-delay: LavinMQ::Shovel::DEFAULT_RECONNECT_DELAY (5 seconds)
ack-mode: LavinMQ::Shovel::DEFAULT_ACK_MODE (on-confirm)
src-delete-after: LavinMQ::Shovel::DEFAULT_DELETE_AFTER (never)

Federation defaults:

prefetch-count: LavinMQ::Federation::Upstream::DEFAULT_PREFETCH (1000)
reconnect-delay: LavinMQ::Federation::Upstream::DEFAULT_RECONNECT_DELAY (1 second)
ack-mode: LavinMQ::Federation::Upstream::DEFAULT_ACK_MODE (on-confirm)
max-hops: LavinMQ::Federation::Upstream::DEFAULT_MAX_HOPS (1)

These default values are pulled directly from the constants defined in Shovel and Upstream classes, ensuring consistency and single source of truth.

Supported Parameters

Shovel parameters:

src-uri (required), dest-uri (required)
src-queue, dest-queue, src-exchange, dest-exchange
src-exchange-key, dest-exchange-key
src-prefetch-count, src-delete-after, ack-mode, reconnect-delay

Federation parameters:

uri (required)
prefetch-count, reconnect-delay, ack-mode, consumer-tag
exchange, queue, max-hops, expires, message-ttl

Fixes #1259.

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot · 2025-09-15T12:36:16Z

@kickster97 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs.

I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review.

- Add PUT and DELETE endpoints for shovels API - Add list_shovels, add_shovel, delete_shovel commands - Add list_federations, add_federation, delete_federation commands - Add comprehensive tests for new shovel endpoints Co-authored-by: kickster97 <[email protected]>

viktorerlingsson · 2025-09-16T08:16:57Z

@copilot you shouldn't add any new endpoints to shovels.cr, the operations can already be done via the parameters endpoint.

Co-authored-by: viktorerlingsson <[email protected]>

Copilot · 2025-09-16T08:24:36Z

@copilot you shouldn't add any new endpoints to shovels.cr, the operations can already be done via the parameters endpoint.

You're absolutely right! I've removed the new PUT and DELETE endpoints from shovels.cr and updated the lavinmqctl commands to use the existing parameters endpoint (/api/parameters/shovel/) instead. This follows the same pattern as federation management and avoids duplicating functionality. The change is in commit b2680a6.

viktorerlingsson · 2025-10-02T09:41:10Z

src/lavinmqctl.cr

+      @parser.on("--trust-user-id", "Trust user ID") do
+        @args["trust-user-id"] = JSON::Any.new(true)
+      end


@copilot what is this? trust-user-id is not a parameter for federation upstreams

You're correct - I've removed the --trust-user-id parameter (which isn't implemented in LavinMQ) and added the actually supported parameters --ack-mode and --consumer-tag that were missing. The federation parameters now match the implementation in upstream_store.cr. Fixed in commit 3feff84.

…nd consumer-tag Co-authored-by: viktorerlingsson <[email protected]>

viktorerlingsson · 2025-10-02T09:48:30Z

@copilot I think we should set some default values for some arguments if they are not provided, for both shovels and federations. Use the default values from ShovelStore and UpstreamStore (for example Upstream::DEFAULT_PREFETCH).

Co-authored-by: viktorerlingsson <[email protected]>

Copilot · 2025-10-02T09:51:19Z

@copilot I think we should set some default values for some arguments if they are not provided, for both shovels and federations. Use the default values from ShovelStore and UpstreamStore (for example...

Added default values matching the constants from ShovelStore and UpstreamStore. Shovel defaults: src-prefetch-count=1000, reconnect-delay=5s, ack-mode=on-confirm, src-delete-after=never. Federation defaults: prefetch-count=1000, reconnect-delay=1s, ack-mode=on-confirm, max-hops=1. Fixed in commit 71e7b22.

viktorerlingsson · 2025-10-02T09:54:56Z

@copilot I think we should set some default values for some arguments if they are not provided, for both shovels and federations. Use the default values from ShovelStore and UpstreamStore (for example...

Added default values matching the constants from ShovelStore and UpstreamStore. Shovel defaults: src-prefetch-count=1000, reconnect-delay=5s, ack-mode=on-confirm, src-delete-after=never. Federation defaults: prefetch-count=1000, reconnect-delay=1s, ack-mode=on-confirm, max-hops=1. Fixed in commit 71e7b22.
Help improve Copilot by leaving feedback using the 👍 or 👎 buttons

@copilot Don't use values for the defaults, use the constants in Upstream and Shovel.

Co-authored-by: viktorerlingsson <[email protected]>

Copilot · 2025-10-02T09:58:19Z

@copilot I think we should set some default values for some arguments if they are not provided, for both shovels and federations. Use the default values from ShovelStore and UpstreamStore (for exa...

Updated to use the actual constants from the classes. Now using LavinMQ::Shovel::DEFAULT_PREFETCH, LavinMQ::Shovel::DEFAULT_RECONNECT_DELAY, etc., and LavinMQ::Federation::Upstream::DEFAULT_PREFETCH, LavinMQ::Federation::Upstream::DEFAULT_RECONNECT_DELAY, etc. Fixed in commit 6f2d8a5.

github-actions · 2025-11-28T08:53:34Z

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

1 out of 3 committers have signed the CLA.
✅ (kickster97)[https://github.com/kickster97]
❌ @Copilot
❌ @carlhoerberg
_{You can retrigger this bot by commenting recheck in this Pull Request.}_{Posted by the CLA Assistant Lite bot.}

claude · 2025-11-28T08:53:41Z

Claude finished @Copilot's task —— View job

Code Review Complete

Read and analyze the changes in src/lavinmqctl.cr
Check for bugs, security vulnerabilities, performance issues
Check for missing error handling and Crystal anti-patterns
Review specific comment about unused code
Provide final review findings

Issues Found

Data Type Mismatch - src/lavinmqctl.cr:897

@args["reconnect-delay"] ||= JSON::Any.new(LavinMQ::Federation::Upstream::DEFAULT_RECONNECT_DELAY.to_i64)

The DEFAULT_RECONNECT_DELAY constant in upstream.cr:14 is defined as an Int32 (value 1), but the code calls .to_i64 on it. While this works, it's unnecessary since JSON::Any can accept Int32 directly.

Inconsistent Time Handling - src/lavinmqctl.cr:860

@args["reconnect-delay"] ||= JSON::Any.new(LavinMQ::Shovel::DEFAULT_RECONNECT_DELAY.total_seconds.to_i64)

Shovel's DEFAULT_RECONNECT_DELAY is a Time::Span (5.seconds), correctly converted to seconds. However, federation's is already an integer representing seconds, so the conversion approaches are inconsistent.

Potential Bug in Enum Conversion - Lines 861 & 898

.to_s.underscore.gsub("_", "-")

The enum-to-string conversion assumes CamelCase enums will convert correctly. Both AckMode::OnConfirm and DeleteAfter::Never should work, but this is fragile if enum naming changes.

No other significant security vulnerabilities, performance issues, or missing error handling found. The unused variable mentioned in the review comment was not found in the current code.

kickster97 · 2025-11-28T08:54:47Z

@viktorerlingsson remembered this since you opened #1350 , gave it some tweaks in the list_shoves command, but other than hat i think it looks alright. wdyt?

kickster97 · 2025-11-28T11:26:59Z

Fixes: 1259

src/lavinmqctl.cr

carlhoerberg · 2025-12-05T22:38:47Z

Do we compile lavinmqctl as part of the CI?

Initial plan

5602ec7

Copilot AI assigned Copilot and kickster97 Sep 15, 2025

Copilot started work on behalf of kickster97 September 15, 2025 12:36 View session

Copilot AI changed the title ~~[WIP] Manage shovels with lavinmqctl~~ Add shovel and federation management commands to lavinmqctl Sep 15, 2025

Copilot finished work on behalf of kickster97 September 15, 2025 12:49

Copilot AI requested a review from kickster97 September 15, 2025 12:49

Copilot started work on behalf of viktorerlingsson September 16, 2025 08:17 View session

Remove new shovel endpoints and use parameters API instead

b2680a6

Co-authored-by: viktorerlingsson <[email protected]>

Copilot finished work on behalf of viktorerlingsson September 16, 2025 08:25

Copilot AI requested a review from viktorerlingsson September 16, 2025 08:25

viktorerlingsson reviewed Oct 2, 2025

View reviewed changes

Copilot started work on behalf of viktorerlingsson October 2, 2025 09:41 View session

Remove unsupported trust-user-id parameter and add missing ack-mode a…

3feff84

…nd consumer-tag Co-authored-by: viktorerlingsson <[email protected]>

Copilot AI requested a review from viktorerlingsson October 2, 2025 09:45

Copilot finished work on behalf of viktorerlingsson October 2, 2025 09:45

Copilot started work on behalf of viktorerlingsson October 2, 2025 09:48 View session

Add default values for shovel and federation parameters

71e7b22

Co-authored-by: viktorerlingsson <[email protected]>

Copilot finished work on behalf of viktorerlingsson October 2, 2025 09:52

Copilot started work on behalf of viktorerlingsson October 2, 2025 09:55 View session

Use constants from Shovel and Upstream classes for default values

6f2d8a5

Co-authored-by: viktorerlingsson <[email protected]>

Copilot finished work on behalf of viktorerlingsson October 2, 2025 09:59

kickster97 removed their assignment Nov 28, 2025

Improve list_shovels to show runtime state and fix compilation

1f252f6

kickster97 marked this pull request as ready for review November 28, 2025 08:53

kickster97 requested a review from a team as a code owner November 28, 2025 08:53

viktorerlingsson approved these changes Nov 28, 2025

View reviewed changes

kickster97 added this to the 2.7.0 milestone Nov 28, 2025

carlhoerberg approved these changes Dec 5, 2025

View reviewed changes

src/lavinmqctl.cr Outdated Show resolved Hide resolved

Remove unused require

1dce799

Add shovel and federation management commands to lavinmqctl #1277

Are you sure you want to change the base?

Add shovel and federation management commands to lavinmqctl #1277

Uh oh!

Conversation

Copilot AI commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Default Values

Supported Parameters

Uh oh!

Copilot AI commented Sep 15, 2025

Uh oh!

viktorerlingsson commented Sep 16, 2025

Uh oh!

Copilot AI commented Sep 16, 2025

Uh oh!

viktorerlingsson Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

viktorerlingsson commented Oct 2, 2025

Uh oh!

Copilot AI commented Oct 2, 2025

Uh oh!

viktorerlingsson commented Oct 2, 2025

Uh oh!

Copilot AI commented Oct 2, 2025

Uh oh!

github-actions bot commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete

Issues Found

Uh oh!

kickster97 commented Nov 28, 2025

Uh oh!

kickster97 commented Nov 28, 2025

Uh oh!

Uh oh!

carlhoerberg commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Sep 15, 2025 •

edited

Loading

github-actions bot commented Nov 28, 2025 •

edited

Loading

claude bot commented Nov 28, 2025 •

edited

Loading