chore(cx-api): automatically determine new feature flag version (aws#…

…22838)

With the new feature flags mechanism, you have to put the `v2` version a flag becomes available in when adding a flag.

One problem: the code hasn't been released yet, so you don't know what version that will be. You can guess it's going to be the next release, but what if the review takes a long time, or a new release was juuuust started by the oncall?

----------------------

Solution: we force you to put in the magic version marker `'V2NEXT'`.

We update the sorting mechanism to sort this always after all v2 versions and before any v3 versions, so it appears at the right place in the report.

At release time, when the actual version number is determined and files are updated and committed back to the repository, we replace the magic marker with the actual version number and regenerate the flag report, so that the actual release version number is put in.

These commits will find their way to the `main` branch when the mergeback commit is created at the end of the release pipeline.

This requires inventing a new `cdk-release` protocol: scripts named `on-bump` will be called as part of creating a release, after the version has been bumped but before the new commit has been created.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

CONTRIBUTING.md

@@ -967,9 +967,13 @@ Adding a new flag looks as follows:
 
 1. Define a new const under
    [cx-api/lib/features.ts](https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/cx-api/lib/features.ts)
-   with the name of the context key that **enables** this new feature (for
+   with the name of the context key that enables this new feature (for
    example, `ENABLE_STACK_NAME_DUPLICATES`). The context key should be in the
    form `module.Type:feature` (e.g. `@aws-cdk/core:enableStackNameDuplicates`).
+    - Set `introducedIn.v2` to the literal string `'V2NEXT'`.
+    - Double negatives should be avoided. If you want to add a flag that disables something that was previously
+      enabled, set `default.v2` to `true` and the `recommendedValue` to `false`. You will need to update
+      a test in `features.test.ts` -- this is okay if you have a good reason.
 2. Use `FeatureFlags.of(construct).isEnabled(cxapi.ENABLE_XXX)` to check if this feature is enabled
    in your code. If it is not defined, revert to the legacy behavior.
 3. Add your feature flag to the `FLAGS` map in

packages/@aws-cdk/cx-api/FEATURE_FLAGS.md

@@ -36,9 +36,9 @@ Flags come in three types:
 | [@aws-cdk/aws-ecs:arnFormatIncludesClusterName](#aws-cdkaws-ecsarnformatincludesclustername) | ARN format used by ECS. In the new ARN format, the cluster name is part of the resource ID. | 2.35.0 | (fix) |
 | [@aws-cdk/aws-apigateway:disableCloudWatchRole](#aws-cdkaws-apigatewaydisablecloudwatchrole) | Make default CloudWatch Role behavior safe for multiple API Gateways in one environment | 2.38.0 | (fix) |
 | [@aws-cdk/core:enablePartitionLiterals](#aws-cdkcoreenablepartitionliterals) | Make ARNs concrete if AWS partition is known | 2.38.0 | (fix) |
-| [@aws-cdk/aws-ecs:disableExplicitDeploymentControllerForCircuitBreaker](#aws-cdkaws-ecsdisableexplicitdeploymentcontrollerforcircuitbreaker) | Avoid setting the "ECS" deployment controller when adding a circuit breaker | 2.51.0 | (fix) |
-| [@aws-cdk/aws-events:eventsTargetQueueSameAccount](#aws-cdkaws-eventseventstargetqueuesameaccount) | Event Rules may only push to encrypted SQS queues in the same account | 2.51.0 | (fix) |
-| [@aws-cdk/aws-iam:standardizedServicePrincipals](#aws-cdkaws-iamstandardizedserviceprincipals) | Use standardized (global) service principals everywhere | 2.51.0 | (fix) |
+| [@aws-cdk/aws-ecs:disableExplicitDeploymentControllerForCircuitBreaker](#aws-cdkaws-ecsdisableexplicitdeploymentcontrollerforcircuitbreaker) | Avoid setting the "ECS" deployment controller when adding a circuit breaker | V2NEXT | (fix) |
+| [@aws-cdk/aws-events:eventsTargetQueueSameAccount](#aws-cdkaws-eventseventstargetqueuesameaccount) | Event Rules may only push to encrypted SQS queues in the same account | V2NEXT | (fix) |
+| [@aws-cdk/aws-iam:standardizedServicePrincipals](#aws-cdkaws-iamstandardizedserviceprincipals) | Use standardized (global) service principals everywhere | V2NEXT | (fix) |
 
 <!-- END table -->
 
@@ -661,7 +661,7 @@ This is a feature flag as the new behavior provides a better default experience
 | Since | Default | Recommended |
 | ----- | ----- | ----- |
 | (not in v1) |  |  |
-| 2.51.0 | `false` | `true` |
+| V2NEXT | `false` | `true` |
 
 
 ### @aws-cdk/aws-events:eventsTargetQueueSameAccount
@@ -676,7 +676,7 @@ always apply, regardless of the value of this flag.
 | Since | Default | Recommended |
 | ----- | ----- | ----- |
 | (not in v1) |  |  |
-| 2.51.0 | `false` | `true` |
+| V2NEXT | `false` | `true` |
 
 
 ### @aws-cdk/aws-iam:standardizedServicePrincipals
@@ -692,7 +692,7 @@ This flag disables use of that exceptions database and always uses the global se
 | Since | Default | Recommended |
 | ----- | ----- | ----- |
 | (not in v1) |  |  |
-| 2.51.0 | `false` | `true` |
+| V2NEXT | `false` | `true` |
 
 
 <!-- END details -->

packages/@aws-cdk/cx-api/build-tools/flag-report.ts

@@ -1,7 +1,10 @@
+/**
+ * Generate FEATURE_FLAGS.md, a report of all current feature flags
+ */
 import { promises as fs } from 'fs';
 import * as path from 'path';
 import * as feats from '../lib/features';
-import { FlagInfo, FlagType } from '../lib/private/flag-modeling';
+import { FlagInfo, FlagType, compareVersions } from '../lib/private/flag-modeling';
 
 async function main() {
   await updateMarkdownFile(path.join(__dirname, '..', 'FEATURE_FLAGS.md'), {
@@ -129,8 +132,8 @@ function flags(pred: (x: FlagInfo) => boolean) {
 
   entries.sort((a, b) => firstCmp(
     // Sort by versions first
-    cmpVersions(a[1].introducedIn.v2, b[1].introducedIn.v2),
-    cmpVersions(a[1].introducedIn.v1, b[1].introducedIn.v1),
+    compareVersions(a[1].introducedIn.v2, b[1].introducedIn.v2),
+    compareVersions(a[1].introducedIn.v1, b[1].introducedIn.v1),
     // Then sort by name
     a[0].localeCompare(b[0])));
 
@@ -203,21 +206,6 @@ async function updateMarkdownFile(filename: string, sections: Record<string, str
   await fs.writeFile(filename, contents, { encoding: 'utf-8' });
 }
 
-function cmpVersions(a: string | undefined, b: string | undefined): number {
-  if (a === undefined && b === undefined) { return 0; }
-  if (a === undefined) { return -1; }
-  if (b === undefined) { return 1; }
-
-  const as = a.split('.').map(x => parseInt(x, 10));
-  const bs = b.split('.').map(x => parseInt(x, 10));
-
-  for (let i = 0; i < Math.min(as.length, bs.length); i++) {
-    if (as[i] < bs[i]) { return -1; }
-    if (as[i] > bs[i]) { return 1; }
-  }
-  return as.length - bs.length;
-}
-
 function firstCmp(...xs: number[]) {
   return xs.find(x => x !== 0) ?? 0;
 }

packages/@aws-cdk/cx-api/build-tools/update-vnext.ts

@@ -0,0 +1,28 @@
+/**
+ * Load the `features.ts` source file, and replace the "V2NEXT" version markers with the actual current version
+ */
+import { promises as fs } from 'fs';
+import * as path from 'path';
+import { MAGIC_V2NEXT } from '../lib/private/flag-modeling';
+
+async function main() {
+  const featuresSourceFile = path.join(__dirname, '..', 'lib', 'features.ts');
+
+  let currentv2: string | undefined = JSON.parse(await fs.readFile(path.join(__dirname, '../../../../version.v2.json'), { encoding: 'utf-8' })).version;
+  currentv2 = currentv2?.match(/^[0-9\.]+/)?.[0]; // Make sure to only retain the actual version number, not any '-rc.X' suffix
+
+  if (!currentv2) {
+    throw new Error('Could not determine current v2 version number');
+  }
+
+  let source = await fs.readFile(featuresSourceFile, { encoding: 'utf-8' });
+  source = source.replace(new RegExp(MAGIC_V2NEXT, 'g'), currentv2);
+
+  await fs.writeFile(featuresSourceFile, source, { encoding: 'utf-8' });
+}
+
+main().catch(e => {
+  // eslint-disable-next-line no-console
+  console.error(e);
+  process.exitCode = 1;
+});

packages/@aws-cdk/cx-api/lib/features.ts

@@ -1,12 +1,25 @@
 import { FlagInfo, FlagType } from './private/flag-modeling';
 
-// --------------------------------------------------------------------------------
+////////////////////////////////////////////////////////////////////////
+//
 // This file defines context keys that enable certain features that are
 // implemented behind a flag in order to preserve backwards compatibility for
 // existing apps. When a new app is initialized through `cdk init`, the CLI will
 // automatically add enable these features by adding them to the generated
 // `cdk.json` file.
 //
+////////////////////////////////////////////////////////////////////////
+//
+//  !!! IMPORTANT !!!
+//
+//  When you introduce a new flag, set its 'introducedIn.v2' value to the literal string
+// 'V2·NEXT', without the dot.
+//
+//  DO NOT USE A VARIABLE. DO NOT DEFINE A CONSTANT. The actual value will be string-replaced at
+//  version bump time.
+//
+////////////////////////////////////////////////////////////////////////
+//
 // There are three types of flags: ApiDefault, BugFix, and VisibleContext flags.
 //
 // - ApiDefault flags: change the behavior or defaults of the construct library. When
@@ -513,7 +526,7 @@ export const FLAGS: Record<string, FlagInfo> = {
       from the same account as the Rule can send messages. If a queue is unencrypted, this restriction will
       always apply, regardless of the value of this flag.
       `,
-    introducedIn: { v2: '2.51.0' },
+    introducedIn: { v2: 'V2NEXT' },
     recommendedValue: true,
   },
 
@@ -527,7 +540,7 @@ export const FLAGS: Record<string, FlagInfo> = {
 
       This flag disables use of that exceptions database and always uses the global service principal.
       `,
-    introducedIn: { v2: '2.51.0' },
+    introducedIn: { v2: 'V2NEXT' },
     recommendedValue: true,
   },
 
@@ -542,7 +555,7 @@ export const FLAGS: Record<string, FlagInfo> = {
 
       This is a feature flag as the new behavior provides a better default experience for the users.
       `,
-    introducedIn: { v2: '2.51.0' },
+    introducedIn: { v2: 'V2NEXT' },
     recommendedValue: true,
   },
 };

packages/@aws-cdk/cx-api/lib/private/flag-modeling.ts

@@ -45,4 +45,34 @@ export type FlagInfo = FlagInfoBase & (
     /** Describe restoring old behavior or dealing with the change (Markdown) */
     readonly compatibilityWithOldBehaviorMd?: string }
   | { readonly type: FlagType.VisibleContext }
-);
+);
+
+/**
+ * The magic value that will be substituted at version bump time with the actual
+ * new V2 version.
+ *
+ * Do not import this constant in the `features.ts` file, or the substitution
+ * process won't work.
+ */
+export const MAGIC_V2NEXT = 'V2NEXT';
+
+/**
+ * Compare two versions, returning -1, 0, or 1.
+ */
+export function compareVersions(a: string | undefined, b: string | undefined): number {
+  if (a === b) { return 0; }
+  if (a === undefined) { return -1; }
+  if (b === undefined) { return 1; }
+
+  const as = a.split('.').map(x => parseInt(x, 10));
+  const bs = b.split('.').map(x => parseInt(x, 10));
+
+  if (a === MAGIC_V2NEXT) { return bs[0] <= 2 ? 1 : -1; }
+  if (b === MAGIC_V2NEXT) { return as[0] <= 2 ? -1 : 1; }
+
+  for (let i = 0; i < Math.min(as.length, bs.length); i++) {
+    if (as[i] < bs[i]) { return -1; }
+    if (as[i] > bs[i]) { return 1; }
+  }
+  return as.length - bs.length;
+}

packages/@aws-cdk/cx-api/package.json

@@ -41,6 +41,7 @@
     "build": "cdk-build",
     "watch": "cdk-watch",
     "lint": "cdk-lint && madge --circular --extensions js lib",
+    "on-bump": "yarn build && node ./build-tools/update-vnext && yarn build && node ./build-tools/flag-report",
     "test": "cdk-test",
     "pkglint": "pkglint -f",
     "package": "cdk-package",

packages/@aws-cdk/cx-api/test/features.test.ts

@@ -1,4 +1,7 @@
+import * as fs from 'fs';
+import * as path from 'path';
 import * as feats from '../lib/features';
+import { MAGIC_V2NEXT, compareVersions } from '../lib/private/flag-modeling';
 
 test('all future flags have defaults configured', () => {
   Object.keys(feats.FLAGS).forEach(flag => {
@@ -19,6 +22,7 @@ test('feature flag defaults may not be changed anymore', () => {
   // In that case, it is permitted to name the flag `oldBehavior`, add a new default set to `true`,
   // and have the recommended value be `false`.
   expect(feats.CURRENT_VERSION_FLAG_DEFAULTS).toEqual({
+    // V1->V2 defaults below here
     [feats.APIGATEWAY_USAGEPLANKEY_ORDERINSENSITIVE_ID]: true,
     [feats.ENABLE_STACK_NAME_DUPLICATES_CONTEXT]: true,
     [feats.ENABLE_DIFF_NO_FAIL_CONTEXT]: true,
@@ -33,6 +37,9 @@ test('feature flag defaults may not be changed anymore', () => {
     [feats.EFS_DEFAULT_ENCRYPTION_AT_REST]: true,
     [feats.LAMBDA_RECOGNIZE_VERSION_PROPS]: true,
     [feats.CLOUDFRONT_DEFAULT_SECURITY_POLICY_TLS_V1_2_2021]: true,
+    // Add new disabling feature flags below this line
+    // ...
+
   });
 });
 
@@ -47,4 +54,38 @@ test('expired feature flags may not be changed anymore', () => {
     feats.S3_GRANT_WRITE_WITHOUT_ACL,
     feats.SECRETS_MANAGER_PARSE_OWNED_SECRET_NAME,
   ].sort());
+});
+
+test.each([
+  ['1.2.3', '1.2.3', 0],
+  ['1.2.3', '1.2.4', -1],
+  ['1.2.3', '2.0.0', -1],
+  ['100.2.3', '2.0.0', 1],
+  ['V2NEXT', 'V2NEXT', 0],
+  ['1.0.0', 'V2NEXT', -1],
+  ['2.100.0', 'V2NEXT', -1],
+  ['3.100.0', 'V2NEXT', 1],
+])('compareVersions(%p, %p) -> %p (and the reverse)', (a, b, expected) => {
+  expect(compareVersions(a, b)).toEqual(expected);
+  expect(compareVersions(b, a)).toBeCloseTo(-expected, 10); // Gets around expect(-0).toEqual(0) failing... :x
+});
+
+const currentv2: string = JSON.parse(fs.readFileSync(path.join(__dirname, '../../../../version.v2.json'), { encoding: 'utf-8' })).version;
+
+describe(`introducedIn.v2 is either <= ${currentv2} or magic value "${MAGIC_V2NEXT}"`, () => {
+  test.each(Object.keys(feats.FLAGS))('for flag %p', flag => {
+    const v2In = feats.FLAGS[flag].introducedIn.v2;
+    if (v2In === undefined || v2In === MAGIC_V2NEXT) {
+      return;
+    }
+
+    // If defined and not magic, it must be in the past w.r.t. the current v2 version
+    expect(compareVersions(v2In, currentv2)).not.toEqual(1);
+  });
+});
+
+test('features.ts should not contain a reference to the constant with the magic value', () => {
+  // If it did, the above test would succeed but we would not be able to substitute the string at bump time
+  const featuresSourceFile = path.join(__dirname, '..', 'lib', 'features.ts');
+  expect(fs.readFileSync(featuresSourceFile, { encoding: 'utf-8' })).not.toContain('MAGIC_V2NEXT');
 });

scripts/bump.js

@@ -24,6 +24,7 @@ async function main() {
     bumpFiles: [ { filename: ver.versionFile, type: 'json' } ],
     infile: ver.changelogFile,
     prerelease: ver.prerelease,
+    repoRoot,
     scripts: {
       postchangelog: `node ${path.join(__dirname, 'changelog-experimental-fix.js')} ${changelogPath}`
     }

tools/@aws-cdk/cdk-release/lib/index.ts

@@ -1,6 +1,7 @@
 import { getConventionalCommitsFromGitHistory } from './conventional-commits';
 import { defaults } from './defaults';
 import { bump } from './lifecycles/bump';
+import { runBumpHooks } from './lifecycles/bumphooks';
 import { writeChangelogs } from './lifecycles/changelog';
 import { commit } from './lifecycles/commit';
 import { debug, debugObject } from './private/print';
@@ -19,6 +20,10 @@ export async function createRelease(opts: ReleaseOptions): Promise<void> {
   };
   debugObject(args, 'options are (including defaults)', args);
 
+  if (!args.repoRoot) {
+    throw new Error('repoRoot is required');
+  }
+
   const currentVersion = readVersion(args.versionFile);
   debugObject(args, 'Current version info', currentVersion);
 
@@ -28,11 +33,20 @@ export async function createRelease(opts: ReleaseOptions): Promise<void> {
   debug(args, 'Reading Git commits');
   const commits = await getConventionalCommitsFromGitHistory(args, `v${currentVersion.stableVersion}`);
 
+  const packages = getProjectPackageInfos();
+
   debug(args, 'Writing Changelog');
-  const changelogResults = await writeChangelogs({ ...args, currentVersion, newVersion, commits, packages: getProjectPackageInfos() });
+  const changelogResults = await writeChangelogs({ ...args, currentVersion, newVersion, commits, packages });
+
+  debug(args, 'Running "on-bump" hooks');
+  const bumpHookedFiles = await runBumpHooks({ ...args, packages });
 
   debug(args, 'Committing result');
-  await commit(args, newVersion.stableVersion, [args.versionFile, ...changelogResults.map(r => r.filePath)]);
+  await commit(args, newVersion.stableVersion, [
+    args.versionFile,
+    ...changelogResults.map(r => r.filePath),
+    ...bumpHookedFiles,
+  ]);
 };
 
 function getProjectPackageInfos(): PackageInfo[] {