Skip to content
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

refactor: move mode checks to getForwardablePorts #5022

Merged
merged 3 commits into from
Sep 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/plugins/kubernetes/helm/deployment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ export const helmDeploy: DeployActionHandler<"deploy", HelmDeployAction> = async
})

// Local mode has its own port-forwarding configuration
const forwardablePorts = mode === "local" && spec.localMode ? [] : getForwardablePorts(manifests, action)
const forwardablePorts = getForwardablePorts({ resources: manifests, parentAction: action, mode })

// Make sure port forwards work after redeployment
killPortForwards(action, forwardablePorts || [], log)
Expand Down
2 changes: 0 additions & 2 deletions core/src/plugins/kubernetes/helm/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ export const helmModuleHandlers: Partial<ModuleActionHandlers<HelmModule>> = {
// sets a `build.dependencies[].copy` directive.

let deployAction: HelmDeployConfig | null = null
let deployDep: string | null = null

// If this Helm module has `skipDeploy = true`, there won't be a service config for us to convert here.
if (service) {
Expand All @@ -66,7 +65,6 @@ export const helmModuleHandlers: Partial<ModuleActionHandlers<HelmModule>> = {
convertBuildDependency,
prepareRuntimeDependencies,
})
deployDep = `deploy.${deployAction.name}`
actions.push(deployAction)
}

Expand Down
2 changes: 1 addition & 1 deletion core/src/plugins/kubernetes/helm/status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export const getHelmDeployStatus: DeployActionHandler<"getStatus", HelmDeployAct
if (state !== "missing") {
const deployedResources = await getDeployedChartResources({ ctx: k8sCtx, action, releaseName, log })

forwardablePorts = deployedMode === "local" ? [] : getForwardablePorts(deployedResources, action)
forwardablePorts = getForwardablePorts({ resources: deployedResources, parentAction: action, mode: deployedMode })
ingresses = getK8sIngresses(deployedResources)

if (state === "ready") {
Expand Down
3 changes: 1 addition & 2 deletions core/src/plugins/kubernetes/kubernetes-type/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,7 @@ export const getKubernetesDeployStatus: DeployActionHandler<"getStatus", Kuberne
log,
})

// Local mode has its own port-forwarding configuration
const forwardablePorts = deployedMode === "local" ? [] : getForwardablePorts(remoteResources, action)
const forwardablePorts = getForwardablePorts({ resources: remoteResources, parentAction: action, mode: deployedMode })

if (state === "ready") {
// Local mode always takes precedence over sync mode
Expand Down
19 changes: 15 additions & 4 deletions core/src/plugins/kubernetes/port-forward.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import { ChildProcess } from "child_process"

import getPort = require("get-port")

const AsyncLock = require("async-lock")
import { V1ContainerPort, V1Deployment, V1PodTemplate, V1Service } from "@kubernetes/client-node"

Expand All @@ -27,7 +28,7 @@ import { KubernetesDeployAction } from "./kubernetes-type/config"
import { HelmDeployAction } from "./helm/config"
import { DeployAction } from "../../actions/deploy"
import { GetPortForwardResult } from "../../plugin/handlers/Deploy/get-port-forward"
import { Resolved } from "../../actions/types"
import { ActionMode, Resolved } from "../../actions/types"

// TODO: implement stopPortForward handler

Expand Down Expand Up @@ -220,12 +221,22 @@ function getTargetResourceName(action: SupportedRuntimeAction, targetName?: stri
/**
* Returns a list of forwardable ports based on the specified resources.
*/
export function getForwardablePorts(
resources: KubernetesResource[],
export function getForwardablePorts({
resources,
parentAction,
mode,
}: {
resources: KubernetesResource[]
parentAction: Resolved<KubernetesDeployAction | HelmDeployAction> | undefined
): ForwardablePort[] {
mode: ActionMode
}): ForwardablePort[] {
const spec = parentAction?.getSpec()

// Note: Local mode has its own port-forwarding configuration
if (mode === "local" && !!spec?.localMode) {
return []
}

if (spec?.portForwards) {
return spec?.portForwards.map((p) => ({
name: p.name,
Expand Down
45 changes: 25 additions & 20 deletions core/test/unit/src/plugins/kubernetes/port-forward.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import { ResolvedDeployAction } from "../../../../../src/actions/deploy"

describe("getForwardablePorts", () => {
it("returns all ports for Service resources", () => {
const ports = getForwardablePorts(
[
const ports = getForwardablePorts({
resources: [
{
apiVersion: "v1",
kind: "Service",
Expand All @@ -29,8 +29,9 @@ describe("getForwardablePorts", () => {
},
},
],
undefined
)
parentAction: undefined,
mode: "default",
})

expect(ports).to.eql([
{
Expand Down Expand Up @@ -63,8 +64,8 @@ describe("getForwardablePorts", () => {
},
}

const ports = getForwardablePorts(
[
const ports = getForwardablePorts({
resources: [
{
apiVersion: "v1",
kind: "Service",
Expand All @@ -76,8 +77,9 @@ describe("getForwardablePorts", () => {
},
},
],
action
)
parentAction: action,
mode: "default",
})

expect(ports).to.eql([
{
Expand All @@ -91,8 +93,8 @@ describe("getForwardablePorts", () => {
})

it("returns all ports for Deployment resources", () => {
const ports = getForwardablePorts(
[
const ports = getForwardablePorts({
resources: [
{
apiVersion: "apps/v1",
kind: "Deployment",
Expand All @@ -112,8 +114,9 @@ describe("getForwardablePorts", () => {
},
},
],
undefined
)
parentAction: undefined,
mode: "default",
})

expect(ports).to.eql([
{
Expand All @@ -126,8 +129,8 @@ describe("getForwardablePorts", () => {
})

it("returns all ports for DaemonSet resources", () => {
const ports = getForwardablePorts(
[
const ports = getForwardablePorts({
resources: [
{
apiVersion: "apps/v1",
kind: "DaemonSet",
Expand All @@ -147,8 +150,9 @@ describe("getForwardablePorts", () => {
},
},
],
undefined
)
parentAction: undefined,
mode: "default",
})

expect(ports).to.eql([
{
Expand All @@ -161,8 +165,8 @@ describe("getForwardablePorts", () => {
})

it("omits a Deployment port that is already pointed to by a Service resource", () => {
const ports = getForwardablePorts(
[
const ports = getForwardablePorts({
resources: [
{
apiVersion: "v1",
kind: "Service",
Expand Down Expand Up @@ -200,8 +204,9 @@ describe("getForwardablePorts", () => {
},
},
],
undefined
)
parentAction: undefined,
mode: "default",
})

expect(ports).to.eql([
{
Expand Down