feat(api): deprecate StartActionByGet* and warn on Diagnostics when called (#1159)

frontend/resources/scripts/gen/olivetin/api/v1/olivetin_pb.d.ts

@@ -2288,7 +2288,10 @@ export declare const OliveTinApiService: GenService<{
     output: typeof StartActionAndWaitResponseSchema;
   },
   /**
+   * Use StartAction instead; this method will be removed. See https://github.com/OliveTin/OliveTin/issues/1028
+   *
    * @generated from rpc olivetin.api.v1.OliveTinApiService.StartActionByGet
+   * @deprecated
    */
   startActionByGet: {
     methodKind: "unary";
@@ -2296,7 +2299,10 @@ export declare const OliveTinApiService: GenService<{
     output: typeof StartActionByGetResponseSchema;
   },
   /**
+   * Use StartActionAndWait instead; this method will be removed. See https://github.com/OliveTin/OliveTin/issues/1028
+   *
    * @generated from rpc olivetin.api.v1.OliveTinApiService.StartActionByGetAndWait
+   * @deprecated
    */
   startActionByGetAndWait: {
     methodKind: "unary";

proto/olivetin/api/v1/olivetin.proto

@@ -524,9 +524,15 @@ service OliveTinApiService {
 
 	rpc StartActionAndWait(StartActionAndWaitRequest) returns (StartActionAndWaitResponse) {}
 
-	rpc StartActionByGet(StartActionByGetRequest) returns (StartActionByGetResponse) {}
-
-	rpc StartActionByGetAndWait(StartActionByGetAndWaitRequest) returns (StartActionByGetAndWaitResponse) {}
+	// Use StartAction instead; this method will be removed. See https://github.com/OliveTin/OliveTin/issues/1028
+	rpc StartActionByGet(StartActionByGetRequest) returns (StartActionByGetResponse) {
+		option deprecated = true;
+	}
+
+	// Use StartActionAndWait instead; this method will be removed. See https://github.com/OliveTin/OliveTin/issues/1028
+	rpc StartActionByGetAndWait(StartActionByGetAndWaitRequest) returns (StartActionByGetAndWaitResponse) {
+		option deprecated = true;
+	}
 
     rpc RestartAction(RestartActionRequest) returns (StartActionResponse) {}
 

service/gen/olivetin/api/v1/apiv1connect/olivetin.connect.go

@@ -117,7 +117,13 @@ type OliveTinApiServiceClient interface {
 	GetDashboard(context.Context, *connect.Request[v1.GetDashboardRequest]) (*connect.Response[v1.GetDashboardResponse], error)
 	StartAction(context.Context, *connect.Request[v1.StartActionRequest]) (*connect.Response[v1.StartActionResponse], error)
 	StartActionAndWait(context.Context, *connect.Request[v1.StartActionAndWaitRequest]) (*connect.Response[v1.StartActionAndWaitResponse], error)
+	// Use StartAction instead; this method will be removed. See https://github.com/OliveTin/OliveTin/issues/1028
+	//
+	// Deprecated: do not use.
 	StartActionByGet(context.Context, *connect.Request[v1.StartActionByGetRequest]) (*connect.Response[v1.StartActionByGetResponse], error)
+	// Use StartActionAndWait instead; this method will be removed. See https://github.com/OliveTin/OliveTin/issues/1028
+	//
+	// Deprecated: do not use.
 	StartActionByGetAndWait(context.Context, *connect.Request[v1.StartActionByGetAndWaitRequest]) (*connect.Response[v1.StartActionByGetAndWaitResponse], error)
 	RestartAction(context.Context, *connect.Request[v1.RestartActionRequest]) (*connect.Response[v1.StartActionResponse], error)
 	KillAction(context.Context, *connect.Request[v1.KillActionRequest]) (*connect.Response[v1.KillActionResponse], error)
@@ -358,11 +364,15 @@ func (c *oliveTinApiServiceClient) StartActionAndWait(ctx context.Context, req *
 }
 
 // StartActionByGet calls olivetin.api.v1.OliveTinApiService.StartActionByGet.
+//
+// Deprecated: do not use.
 func (c *oliveTinApiServiceClient) StartActionByGet(ctx context.Context, req *connect.Request[v1.StartActionByGetRequest]) (*connect.Response[v1.StartActionByGetResponse], error) {
 	return c.startActionByGet.CallUnary(ctx, req)
 }
 
 // StartActionByGetAndWait calls olivetin.api.v1.OliveTinApiService.StartActionByGetAndWait.
+//
+// Deprecated: do not use.
 func (c *oliveTinApiServiceClient) StartActionByGetAndWait(ctx context.Context, req *connect.Request[v1.StartActionByGetAndWaitRequest]) (*connect.Response[v1.StartActionByGetAndWaitResponse], error) {
 	return c.startActionByGetAndWait.CallUnary(ctx, req)
 }
@@ -477,7 +487,13 @@ type OliveTinApiServiceHandler interface {
 	GetDashboard(context.Context, *connect.Request[v1.GetDashboardRequest]) (*connect.Response[v1.GetDashboardResponse], error)
 	StartAction(context.Context, *connect.Request[v1.StartActionRequest]) (*connect.Response[v1.StartActionResponse], error)
 	StartActionAndWait(context.Context, *connect.Request[v1.StartActionAndWaitRequest]) (*connect.Response[v1.StartActionAndWaitResponse], error)
+	// Use StartAction instead; this method will be removed. See https://github.com/OliveTin/OliveTin/issues/1028
+	//
+	// Deprecated: do not use.
 	StartActionByGet(context.Context, *connect.Request[v1.StartActionByGetRequest]) (*connect.Response[v1.StartActionByGetResponse], error)
+	// Use StartActionAndWait instead; this method will be removed. See https://github.com/OliveTin/OliveTin/issues/1028
+	//
+	// Deprecated: do not use.
 	StartActionByGetAndWait(context.Context, *connect.Request[v1.StartActionByGetAndWaitRequest]) (*connect.Response[v1.StartActionByGetAndWaitResponse], error)
 	RestartAction(context.Context, *connect.Request[v1.RestartActionRequest]) (*connect.Response[v1.StartActionResponse], error)
 	KillAction(context.Context, *connect.Request[v1.KillActionRequest]) (*connect.Response[v1.KillActionResponse], error)

service/gen/olivetin/api/v1/olivetin.pb.go

@@ -5557,13 +5557,13 @@ const file_olivetin_api_v1_olivetin_proto_rawDesc = "" +
 	"unique_key\x18\x01 \x01(\tR\tuniqueKey\x12\x12\n" +
 	"\x04type\x18\x02 \x01(\tR\x04type\"J\n" +
 	"\x14RestartActionRequest\x122\n" +
-	"\x15execution_tracking_id\x18\x01 \x01(\tR\x13executionTrackingId2\xee\x13\n" +
+	"\x15execution_tracking_id\x18\x01 \x01(\tR\x13executionTrackingId2\xf5\x13\n" +
 	"\x12OliveTinApiService\x12]\n" +
 	"\fGetDashboard\x12$.olivetin.api.v1.GetDashboardRequest\x1a%.olivetin.api.v1.GetDashboardResponse\"\x00\x12Z\n" +
 	"\vStartAction\x12#.olivetin.api.v1.StartActionRequest\x1a$.olivetin.api.v1.StartActionResponse\"\x00\x12o\n" +
-	"\x12StartActionAndWait\x12*.olivetin.api.v1.StartActionAndWaitRequest\x1a+.olivetin.api.v1.StartActionAndWaitResponse\"\x00\x12i\n" +
-	"\x10StartActionByGet\x12(.olivetin.api.v1.StartActionByGetRequest\x1a).olivetin.api.v1.StartActionByGetResponse\"\x00\x12~\n" +
-	"\x17StartActionByGetAndWait\x12/.olivetin.api.v1.StartActionByGetAndWaitRequest\x1a0.olivetin.api.v1.StartActionByGetAndWaitResponse\"\x00\x12^\n" +
+	"\x12StartActionAndWait\x12*.olivetin.api.v1.StartActionAndWaitRequest\x1a+.olivetin.api.v1.StartActionAndWaitResponse\"\x00\x12l\n" +
+	"\x10StartActionByGet\x12(.olivetin.api.v1.StartActionByGetRequest\x1a).olivetin.api.v1.StartActionByGetResponse\"\x03\x88\x02\x01\x12\x81\x01\n" +
+	"\x17StartActionByGetAndWait\x12/.olivetin.api.v1.StartActionByGetAndWaitRequest\x1a0.olivetin.api.v1.StartActionByGetAndWaitResponse\"\x03\x88\x02\x01\x12^\n" +
 	"\rRestartAction\x12%.olivetin.api.v1.RestartActionRequest\x1a$.olivetin.api.v1.StartActionResponse\"\x00\x12W\n" +
 	"\n" +
 	"KillAction\x12\".olivetin.api.v1.KillActionRequest\x1a#.olivetin.api.v1.KillActionResponse\"\x00\x12f\n" +

service/internal/api/api.go

@@ -336,6 +336,8 @@ func (api *oliveTinAPI) StartActionAndWait(ctx ctx.Context, req *connect.Request
 }
 
 func (api *oliveTinAPI) StartActionByGet(ctx ctx.Context, req *connect.Request[apiv1.StartActionByGetRequest]) (*connect.Response[apiv1.StartActionByGetResponse], error) {
+	reportDeprecatedAPICall("StartActionByGet", "StartAction")
+
 	binding := api.executor.FindBindingByID(req.Msg.ActionId)
 	if binding == nil || binding.Action == nil {
 		return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", req.Msg.ActionId))
@@ -387,6 +389,8 @@ func (api *oliveTinAPI) startActionByGetAndWaitLogEntry(binding *executor.Action
 }
 
 func (api *oliveTinAPI) StartActionByGetAndWait(ctx ctx.Context, req *connect.Request[apiv1.StartActionByGetAndWaitRequest]) (*connect.Response[apiv1.StartActionByGetAndWaitResponse], error) {
+	reportDeprecatedAPICall("StartActionByGetAndWait", "StartActionAndWait")
+
 	binding := api.executor.FindBindingByID(req.Msg.ActionId)
 	if binding == nil || binding.Action == nil {
 		return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("action with ID %s not found", req.Msg.ActionId))

service/internal/api/api_deprecated.go

@@ -0,0 +1,21 @@
+package api
+
+import (
+	"fmt"
+
+	"github.com/OliveTin/OliveTin/internal/configissues"
+)
+
+// reportDeprecatedAPICall raises a warning on the Diagnostics page (once per
+// method, until OliveTin restarts) when a deprecated API method is called, so
+// admins can find and update the integrations still using it.
+func reportDeprecatedAPICall(method, replacement string) {
+	configissues.ReportRuntime(configissues.Issue{
+		Severity: configissues.SeverityWarning,
+		Code:     configissues.CodeDeprecatedAPICall,
+		Message: fmt.Sprintf(
+			"The deprecated API method %s was called. Use %s instead, as %s will be removed in a future version. See https://docs.olivetin.app/api/start_action.html#api-deprecated-byget",
+			method, replacement, method),
+		Source: method,
+	})
+}

service/internal/api/api_deprecated_test.go

@@ -0,0 +1,82 @@
+package api
+
+import (
+	"context"
+	"testing"
+
+	"connectrpc.com/connect"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	apiv1 "github.com/OliveTin/OliveTin/gen/olivetin/api/v1"
+	config "github.com/OliveTin/OliveTin/internal/config"
+	"github.com/OliveTin/OliveTin/internal/configissues"
+	"github.com/OliveTin/OliveTin/internal/executor"
+)
+
+func deprecatedAPICallSources(t *testing.T, client interface {
+	GetDiagnostics(context.Context, *connect.Request[apiv1.GetDiagnosticsRequest]) (*connect.Response[apiv1.GetDiagnosticsResponse], error)
+}) map[string]bool {
+	t.Helper()
+
+	res, err := client.GetDiagnostics(context.Background(), connect.NewRequest(&apiv1.GetDiagnosticsRequest{}))
+	require.NoError(t, err)
+
+	sources := map[string]bool{}
+	for _, issue := range res.Msg.ConfigIssues {
+		if issue.Code == configissues.CodeDeprecatedAPICall {
+			assert.Equal(t, configissues.SeverityWarning, issue.Severity)
+			sources[issue.Source] = true
+		}
+	}
+	return sources
+}
+
+func TestDeprecatedStartActionByGetRaisesDiagnosticsWarning(t *testing.T) {
+	configissues.ClearRuntime()
+	t.Cleanup(configissues.ClearRuntime)
+
+	cfg := config.DefaultConfig()
+	cfg.Actions = []*config.Action{{Title: "Ping", ID: "ping", Shell: "echo ping"}}
+
+	ex := executor.DefaultExecutor(cfg)
+	ex.RebuildActionMap()
+	testServer, client := getNewTestServerAndClientWithExecutor(cfg, ex)
+	defer testServer.Close()
+
+	assert.Empty(t, deprecatedAPICallSources(t, client))
+
+	//nolint:staticcheck // Deliberately calling the deprecated RPC.
+	_, err := client.StartActionByGet(context.Background(), connect.NewRequest(&apiv1.StartActionByGetRequest{ActionId: "ping"}))
+	require.NoError(t, err)
+
+	//nolint:staticcheck // Deliberately calling the deprecated RPC.
+	_, err = client.StartActionByGetAndWait(context.Background(), connect.NewRequest(&apiv1.StartActionByGetAndWaitRequest{ActionId: "ping"}))
+	require.NoError(t, err)
+
+	sources := deprecatedAPICallSources(t, client)
+	assert.True(t, sources["StartActionByGet"])
+	assert.True(t, sources["StartActionByGetAndWait"])
+
+	// Config reloads and entity changes rebuild the issue list; the warnings must survive.
+	ex.RebuildActionMap()
+	sources = deprecatedAPICallSources(t, client)
+	assert.True(t, sources["StartActionByGet"])
+	assert.True(t, sources["StartActionByGetAndWait"])
+}
+
+func TestStartActionDoesNotRaiseDeprecatedWarning(t *testing.T) {
+	configissues.ClearRuntime()
+	t.Cleanup(configissues.ClearRuntime)
+
+	cfg := config.DefaultConfig()
+	cfg.Actions = []*config.Action{{Title: "Ping", ID: "ping", Shell: "echo ping"}}
+
+	testServer, client := getNewTestServerAndClient(cfg)
+	defer testServer.Close()
+
+	_, err := client.StartAction(context.Background(), connect.NewRequest(&apiv1.StartActionRequest{BindingId: "ping"}))
+	require.NoError(t, err)
+
+	assert.Empty(t, deprecatedAPICallSources(t, client))
+}

service/internal/configcheck/rebuild.go

@@ -17,16 +17,17 @@ import (
 )
 
 // Rebuild clears and rebuilds the issue list from the current config, entity
-// state, sticky load-time reports, and any extra issues provided by callers
+// state, sticky load-time reports, runtime reports, and any extra issues provided by callers
 // (for example literal argument default validation from the executor).
 func Rebuild(cfg *config.Config, extra ...configissues.Issue) {
 	if cfg == nil {
-		configissues.Replace(extra)
+		configissues.Replace(append(configissues.CopyRuntime(), extra...))
 		return
 	}
 
 	collected := make([]configissues.Issue, 0)
 	collected = append(collected, configissues.CopySticky()...)
+	collected = append(collected, configissues.CopyRuntime()...)
 	collected = append(collected, collectActionGroupIssues(cfg)...)
 	collected = append(collected, collectAclReferenceIssues(cfg)...)
 	collected = append(collected, collectArgumentIssues(cfg)...)

service/internal/configissues/issue.go

@@ -22,6 +22,7 @@ const (
 	CodeCronEntityBinding      = "cron_entity_binding"
 	CodeWatcherPath            = "watcher_path"
 	CodeAclUnknown             = "acl_unknown"
+	CodeDeprecatedAPICall      = "deprecated_api_call"
 )
 
 // Issue is a configuration warning or error surfaced on Diagnostics.

service/internal/configissues/runtime.go

@@ -0,0 +1,47 @@
+package configissues
+
+import "sync"
+
+var (
+	runtimeMu     sync.Mutex
+	runtimeIssues []Issue
+)
+
+// ReportRuntime records an issue discovered while OliveTin is running, rather
+// than from the configuration (for example, a call to a deprecated API
+// method). Unlike Report, runtime issues are kept when the issue list is
+// rebuilt on config reload or entity changes. They are cleared on restart.
+func ReportRuntime(issue Issue) {
+	addRuntime(issue)
+	Report(issue)
+}
+
+func addRuntime(issue Issue) {
+	runtimeMu.Lock()
+	defer runtimeMu.Unlock()
+
+	fingerprint := issueFingerprint(issue)
+	for _, existing := range runtimeIssues {
+		if issueFingerprint(existing) == fingerprint {
+			return
+		}
+	}
+
+	runtimeIssues = append(runtimeIssues, issue)
+}
+
+// CopyRuntime returns a copy of the runtime issues.
+func CopyRuntime() []Issue {
+	runtimeMu.Lock()
+	defer runtimeMu.Unlock()
+	out := make([]Issue, len(runtimeIssues))
+	copy(out, runtimeIssues)
+	return out
+}
+
+// ClearRuntime removes all runtime issues. Intended for tests.
+func ClearRuntime() {
+	runtimeMu.Lock()
+	defer runtimeMu.Unlock()
+	runtimeIssues = nil
+}

service/internal/configissues/runtime_test.go

@@ -0,0 +1,30 @@
+package configissues_test
+
+import (
+	"testing"
+
+	"github.com/OliveTin/OliveTin/internal/configissues"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestReportRuntimeDeduplicatesAndLists(t *testing.T) {
+	configissues.ClearRuntime()
+	configissues.Replace(nil)
+	t.Cleanup(func() {
+		configissues.ClearRuntime()
+		configissues.Replace(nil)
+	})
+
+	issue := configissues.Issue{
+		Severity: configissues.SeverityWarning,
+		Code:     configissues.CodeDeprecatedAPICall,
+		Message:  "deprecated",
+		Source:   "StartActionByGet",
+	}
+
+	configissues.ReportRuntime(issue)
+	configissues.ReportRuntime(issue)
+
+	assert.Len(t, configissues.CopyRuntime(), 1)
+	assert.Equal(t, 1, configissues.Count())
+}