-
Notifications
You must be signed in to change notification settings - Fork 4
SP-1383: gate early-access commands behind feature flag, replace beta #393
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
base: main
Are you sure you want to change the base?
Changes from all commits
5c19af6
fca9d9c
b0ddbc7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,3 +15,6 @@ npm-debug.log | |
| .idea | ||
| *.iml | ||
| .DS_Store | ||
|
|
||
| # Test coverage output | ||
| coverage/ | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,9 @@ | ||
| import path = require("path"); | ||
| import * as fs from "fs"; | ||
| import { Command, CommandOptions, Option, OptionValues } from "commander"; | ||
| import { Command, CommandOptions, OptionValues } from "commander"; | ||
| import { Context } from "./cli-context"; | ||
| import { GracefulError, logger } from "../utils/logger"; | ||
| import * as chalk from "chalk"; | ||
| import { FeatureFlagService } from "../feature-flag/feature-flag.service"; | ||
|
|
||
| export abstract class IModule { | ||
| public abstract register(context: Context, commandConfig: Configurator): void; | ||
|
|
@@ -186,13 +186,6 @@ export class CommandConfig { | |
| return this; | ||
| } | ||
|
|
||
| public betaOption(flags: string, description?: string, defaultValue?: string | boolean | string[]): CommandConfig { | ||
| const option = new Option(flags, description).default(defaultValue); | ||
| (option as any).isBeta = true; | ||
| this.cmd.addOption(option); | ||
| return this; | ||
| } | ||
|
|
||
| public requiredOption(flags: string, description?: string, defaultValue?: string | boolean | string[]): CommandConfig { | ||
| this.cmd.requiredOption(flags, description, defaultValue); | ||
| return this; | ||
|
|
@@ -203,17 +196,24 @@ export class CommandConfig { | |
| return this; | ||
| } | ||
|
|
||
| public beta(): CommandConfig { | ||
| (this.cmd as any).isBeta = true; | ||
| /** | ||
| * Marks a command as early access (internal, not shown in help). With a feature | ||
| * key, the command is gated at run time on that backend flag. | ||
| */ | ||
| public earlyAccess(featureKey?: string): this { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Zooming out from deployments: collapsing |
||
| const command = this.cmd as any | ||
| command.isEarlyAccess = true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small thing that might trip us up later: (nit: missing semicolon after |
||
| if (featureKey) { | ||
| command.earlyAccessFeatureKey = featureKey; | ||
| } | ||
| return this; | ||
| } | ||
|
|
||
| public action(handler: CommandHandler): void { | ||
| this.cmd.action(async (): Promise<void> => { | ||
| try { | ||
| this.printBetaNoticeIfBetaCommand(); | ||
| this.printBetaNoticeIfBetaOptions(); | ||
| this.printDeprecationNoticeIfDeprecated(); | ||
| await this.checkEarlyAccessFeatureFlag(); | ||
| await handler(this.ctx, this.cmd, this.cmd.optsWithGlobals()); | ||
| } catch (error) { | ||
| if (error instanceof GracefulError) { | ||
|
|
@@ -232,22 +232,30 @@ export class CommandConfig { | |
| } | ||
| } | ||
|
|
||
| private printBetaNoticeIfBetaCommand(): void { | ||
| if ((this.cmd as any).isBeta) { | ||
| logger.info(chalk.yellow("This command is in beta and may change in future releases.")); | ||
| } | ||
| } | ||
|
|
||
| private printBetaNoticeIfBetaOptions(): void { | ||
| if ((this.cmd as any).isBeta) { | ||
| /** Blocks an early-access command (one with a feature key) unless its flag is enabled for the team. */ | ||
| private async checkEarlyAccessFeatureFlag(): Promise<void> { | ||
| const featureKey = (this.cmd as any).earlyAccessFeatureKey; | ||
| if (!featureKey) { | ||
| return; | ||
| } | ||
| const providedOptions = this.cmd.optsWithGlobals(); | ||
| for (const option of this.cmd.options) { | ||
| const optionName = option.attributeName(); | ||
| if ((option as any).isBeta && Object.hasOwn(providedOptions, optionName)) { | ||
| logger.info(chalk.yellow(`The option '${option.long}' is in beta and may change in future releases.`)); | ||
|
|
||
| let enabled: boolean; | ||
| try { | ||
| enabled = await new FeatureFlagService(this.ctx).isEnabled(featureKey); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bigger-picture question on the approach: this turns the CLI into a second source of truth for "is this command allowed". It reads |
||
| } catch (error) { | ||
| // No profile: let the standard "no profile provided" error surface instead of masking it. | ||
| if (!this.ctx.profile) { | ||
| throw error; | ||
|
tneum-celonis marked this conversation as resolved.
|
||
| } | ||
| throw new GracefulError( | ||
| `Could not verify whether '${this.cmd.name()}' is enabled for your team. Please try again later or contact support.` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A couple of edge cases worth thinking through here:
|
||
| ); | ||
| } | ||
|
|
||
| if (!enabled) { | ||
| throw new GracefulError( | ||
| `'${this.cmd.name()}' is not enabled for your team. Contact support to request access.` | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| /** Backend feature-flag keys for early-access commands (passed to `earlyAccess(...)`). */ | ||
| export const EarlyAccessFeature = { | ||
| BRANCHING: "pacman.branching", | ||
| } as const; | ||
|
|
||
| export type EarlyAccessFeatureKey = (typeof EarlyAccessFeature)[keyof typeof EarlyAccessFeature]; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import { Context } from "../command/cli-context"; | ||
| import { HttpClient } from "../http/http-client"; | ||
|
|
||
| interface TeamFeature { | ||
| key: string; | ||
| } | ||
|
|
||
| /** | ||
| * Checks whether a backend feature flag is enabled for the current team via | ||
| * `GET /api/team/features` — a flag is enabled when its key is in the returned list. | ||
| */ | ||
| export class FeatureFlagService { | ||
| private static readonly TEAM_FEATURES_URL = "/api/team/features"; | ||
|
|
||
| private readonly httpClient: () => HttpClient; | ||
|
|
||
| constructor(context: Context) { | ||
| this.httpClient = () => context.httpClient; | ||
| } | ||
|
|
||
| public async isEnabled(featureKey: string): Promise<boolean> { | ||
| const features: TeamFeature[] = await this.httpClient().get(FeatureFlagService.TEAM_FEATURES_URL); | ||
| return Array.isArray(features) && features.some(feature => feature?.key === featureKey); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import Module = require("../../../src/commands/action-flows/module"); | ||
| import { testContext } from "../../utls/test-context"; | ||
| import { createMockConfigurator } from "../../utls/configurator-mock"; | ||
|
|
||
| describe("Action Flows Module", () => { | ||
| describe("register", () => { | ||
| it("registers the action-flows command groups without throwing", () => { | ||
| const mockConfigurator = createMockConfigurator(); | ||
|
|
||
| expect(() => new Module().register(testContext, mockConfigurator)).not.toThrow(); | ||
|
|
||
| expect(mockConfigurator.command).toHaveBeenCalledWith("analyze"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("export"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("import"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("pull"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("push"); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import Module = require("../../../src/commands/data-pipeline/module"); | ||
| import { testContext } from "../../utls/test-context"; | ||
| import { createMockConfigurator } from "../../utls/configurator-mock"; | ||
|
|
||
| describe("Data Pipeline Module", () => { | ||
| describe("register", () => { | ||
| it("registers the data-pool and connection command groups without throwing", () => { | ||
| const mockConfigurator = createMockConfigurator(); | ||
|
|
||
| expect(() => new Module().register(testContext, mockConfigurator)).not.toThrow(); | ||
|
|
||
| // data-pool commands | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("export"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("import"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("list"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("pull"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("push"); | ||
| // connection commands | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("get"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("set"); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import Module = require("../../../src/commands/deployment/module"); | ||
| import { testContext } from "../../utls/test-context"; | ||
| import { createMockConfigurator } from "../../utls/configurator-mock"; | ||
|
|
||
| describe("Deployment Module", () => { | ||
| describe("register", () => { | ||
| it("registers the deployment command groups without throwing", () => { | ||
| const mockConfigurator = createMockConfigurator(); | ||
|
|
||
| expect(() => new Module().register(testContext, mockConfigurator)).not.toThrow(); | ||
|
|
||
| expect(mockConfigurator.command).toHaveBeenCalledWith("deployment"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("create"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("list"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("history"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("active"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("deployables"); | ||
| expect(mockConfigurator.command).toHaveBeenCalledWith("targets"); | ||
| }); | ||
|
|
||
| it("marks every deployment command as early access", () => { | ||
| const mockConfigurator = createMockConfigurator(); | ||
|
|
||
| new Module().register(testContext, mockConfigurator); | ||
|
|
||
| // deployment, create, list, history, active, deployables, targets | ||
| const expectedEarlyAccessCommands = 7; | ||
| expect(mockConfigurator.earlyAccess).toHaveBeenCalledTimes(expectedEarlyAccessCommands); | ||
| }); | ||
|
|
||
| it("wires an action handler for every leaf subcommand", () => { | ||
| const mockConfigurator = createMockConfigurator(); | ||
|
|
||
| new Module().register(testContext, mockConfigurator); | ||
|
|
||
| // create, history, active, deployables, targets | ||
| const expectedLeafCommands = 5; | ||
| expect(mockConfigurator.action).toHaveBeenCalledTimes(expectedLeafCommands); | ||
| for (const call of mockConfigurator.action.mock.calls) { | ||
| expect(typeof call[0]).toBe("function"); | ||
| } | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, the deployment feature is GA, so this
beta()marker was already a leftover, and moving it off here is the right cleanup. One thing still hanging around:docs/user-guide/deployment-commands.mdis titled "Deployment Commands (beta)", so the docs and the CLI now disagree. We know fully removing the beta tag isn't the point of this PR, so feel free to leave it for a followup PR if it's too much hassle to fold in here.