Include review feedback

Signed-off-by: Eduardo Apolinario <[email protected]>
flyteorg · Jun 13, 2024 · 501d3ee · 501d3ee
1 parent 04facbd
commit 501d3ee
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 9 deletions.
diff --git a/flyteadmin/pkg/runtime/interfaces/application_configuration.go b/flyteadmin/pkg/runtime/interfaces/application_configuration.go
@@ -105,7 +105,7 @@ type ApplicationConfig struct {
 	FeatureGates FeatureGates `json:"featureGates" pflag:",Enable experimental features."`
 
 	// A URL pointing to the flyteconsole instance used to hit this flyteadmin instance.
-	ConsoleURL string `json:"consoleUrl,omitempty"`
+	ConsoleURL string `json:"consoleUrl,omitempty" pflag:",A URL pointing to the flyteconsole instance used to hit this flyteadmin instance."`
 }
 
 func (a *ApplicationConfig) GetRoleNameKey() string {

diff --git a/flyteadmin/pkg/workflowengine/impl/k8s_executor.go b/flyteadmin/pkg/workflowengine/impl/k8s_executor.go
@@ -55,8 +55,8 @@ func (e K8sWorkflowExecutor) Execute(ctx context.Context, data interfaces.Execut
 		flyteWf.Tasks = nil
 	}
 
-	if e.config.ApplicationConfiguration().GetTopLevelConfig().ConsoleURL != "" {
-		flyteWf.ConsoleURL = e.config.ApplicationConfiguration().GetTopLevelConfig().ConsoleURL
+	if consoleURL := e.config.ApplicationConfiguration().GetTopLevelConfig().ConsoleURL; len(consoleURL) > 0 {
+		flyteWf.ConsoleURL = consoleURL
 	}
 
 	executionTargetSpec := executioncluster.ExecutionTargetSpec{

diff --git a/flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds.go b/flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"strconv"
+	"strings"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -14,6 +15,10 @@ import (
 	"github.com/flyteorg/flyte/flytestdlib/contextutils"
 )
 
+const (
+	flyteExecutionURL = "FLYTE_EXECUTION_URL"
+)
+
 func GetContextEnvVars(ownerCtx context.Context) []v1.EnvVar {
 	var envVars []v1.EnvVar
 
@@ -70,10 +75,10 @@ func GetExecutionEnvVars(id pluginsCore.TaskExecutionID, consoleURL string) []v1
 		// },
 	}
 
-	if consoleURL != "" {
+	if len(consoleURL) > 0 {
+		consoleURL = strings.TrimRight(consoleURL, "/")
 		envVars = append(envVars, v1.EnvVar{
-			Name: "FLYTE_EXECUTION_URL",
-			// TODO: should we use net/url to build this url?
+			Name:  flyteExecutionURL,
 			Value: fmt.Sprintf("%s/projects/%s/domains/%s/executions/%s/nodeId/%s/nodes", consoleURL, nodeExecutionID.Project, nodeExecutionID.Domain, nodeExecutionID.Name, id.GetUniqueNodeID()),
 		})
 	}

diff --git a/flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds_test.go b/flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds_test.go
@@ -40,6 +40,24 @@ func TestGetExecutionEnvVars(t *testing.T) {
 				Value: "scheme://host/path/projects/proj/domains/domain/executions/name/nodeId/unique-node-id/nodes",
 			},
 		},
+		{
+			"with-console-url-ending-in-single-slash",
+			13,
+			"scheme://host/path/",
+			&v12.EnvVar{
+				Name:  "FLYTE_EXECUTION_URL",
+				Value: "scheme://host/path/projects/proj/domains/domain/executions/name/nodeId/unique-node-id/nodes",
+			},
+		},
+		{
+			"with-console-url-ending-in-multiple-slashes",
+			13,
+			"scheme://host/path////",
+			&v12.EnvVar{
+				Name:  "FLYTE_EXECUTION_URL",
+				Value: "scheme://host/path/projects/proj/domains/domain/executions/name/nodeId/unique-node-id/nodes",
+			},
+		},
 	}
 	for _, tt := range tests {
 		envVars := GetExecutionEnvVars(mock, tt.consoleURL)

diff --git a/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper.go b/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper.go
@@ -316,7 +316,7 @@ func BuildRawPod(ctx context.Context, tCtx pluginsCore.TaskExecutionContext) (*v
 	return podSpec, &objectMeta, primaryContainerName, nil
 }
 
-func shouldIncludeConsoleURL(taskTemplate *core.TaskTemplate) bool {
+func hasExternalLinkType(taskTemplate *core.TaskTemplate) bool {
 	if taskTemplate == nil {
 		return false
 	}
@@ -345,7 +345,7 @@ func ApplyFlytePodConfiguration(ctx context.Context, tCtx pluginsCore.TaskExecut
 		OutputPath:        tCtx.OutputWriter(),
 		Task:              tCtx.TaskReader(),
 		TaskExecMetadata:  tCtx.TaskExecutionMetadata(),
-		IncludeConsoleURL: shouldIncludeConsoleURL(taskTemplate),
+		IncludeConsoleURL: hasExternalLinkType(taskTemplate),
 	}
 
 	resourceRequests := make([]v1.ResourceRequirements, 0, len(podSpec.Containers))

diff --git a/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper_test.go b/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper_test.go
@@ -2149,7 +2149,7 @@ func TestAddFlyteCustomizationsToContainer_SetConsoleUrl(t *testing.T) {
 			includeConsoleURL: true,
 			consoleURL:        "gopher://flyte:65535/console",
 			expectedEnvVar: &v1.EnvVar{
-				Name:  "FLYTE_EXECUTION_URL",
+				Name:  flyteExecutionURL,
 				Value: "gopher://flyte:65535/console/projects/p2/domains/d2/executions/n2/nodeId/unique_node_id/nodes",
 			},
 		},