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

(neptune-alpha): Fix construct to dynamically update supported engine versions #33444

Open
1 of 2 tasks
triggan opened this issue Feb 13, 2025 · 3 comments
Open
1 of 2 tasks
Labels
@aws-cdk/aws-neptune Related Amazon Neptune effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p3

Comments

@triggan
Copy link

triggan commented Feb 13, 2025

Describe the feature

The construct does not automatically update the supported engine versions and supported instance families. It needs to be altered to fetch the current supported engine versions via: DescribeDBEngineVersionsCommand

And the latest set of supported instance types/sizes via: DescribeOrderableDBInstanceOptionsCommand

Use Case

Provide users the ability to configure latest engine versions for a cluster along with the full list of supported instance types when using the neptune L2 construct (alpha).

Proposed Solution

No response

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.178.2

Environment details (OS name and version, etc.)

N/A

@triggan triggan added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Feb 13, 2025
@github-actions github-actions bot added the @aws-cdk/aws-neptune Related Amazon Neptune label Feb 13, 2025
@pahud
Copy link
Contributor

pahud commented Feb 14, 2025

Thank you for the feedback. I think supported list should be a static enum or enum-like class. For anything newly supported but not yet in the list, CDK should allow user to specify using a static method like of().

Looking at

export class EngineVersion {
/**
* Neptune engine version 1.0.1.0
*/
public static readonly V1_0_1_0 = new EngineVersion('1.0.1.0');
/**
* Neptune engine version 1.0.1.1
*/
public static readonly V1_0_1_1 = new EngineVersion('1.0.1.1');
/**
* Neptune engine version 1.0.1.2
*/
public static readonly V1_0_1_2 = new EngineVersion('1.0.1.2');
/**
* Neptune engine version 1.0.2.1
*/
public static readonly V1_0_2_1 = new EngineVersion('1.0.2.1');
/**
* Neptune engine version 1.0.2.2
*/
public static readonly V1_0_2_2 = new EngineVersion('1.0.2.2');
/**
* Neptune engine version 1.0.3.0
*/
public static readonly V1_0_3_0 = new EngineVersion('1.0.3.0');
/**
* Neptune engine version 1.0.4.0
*/
public static readonly V1_0_4_0 = new EngineVersion('1.0.4.0');
/**
* Neptune engine version 1.0.4.1
*/
public static readonly V1_0_4_1 = new EngineVersion('1.0.4.1');
/**
* Neptune engine version 1.0.5.0
*/
public static readonly V1_0_5_0 = new EngineVersion('1.0.5.0');
/**
* Neptune engine version 1.1.0.0
*/
public static readonly V1_1_0_0 = new EngineVersion('1.1.0.0');
/**
* Neptune engine version 1.1.1.0
*/
public static readonly V1_1_1_0 = new EngineVersion('1.1.1.0');
/**
* Neptune engine version 1.2.0.0
*/
public static readonly V1_2_0_0 = new EngineVersion('1.2.0.0');
/**
* Neptune engine version 1.2.0.1
*/
public static readonly V1_2_0_1 = new EngineVersion('1.2.0.1');
/**
* Neptune engine version 1.2.0.2
*/
public static readonly V1_2_0_2 = new EngineVersion('1.2.0.2');
/**
* Neptune engine version 1.2.1.0
*/
public static readonly V1_2_1_0 = new EngineVersion('1.2.1.0');
/**
* Neptune engine version 1.2.1.1
*/
public static readonly V1_2_1_1 = new EngineVersion('1.2.1.1');
/**
* Neptune engine version 1.2.1.2
*/
public static readonly V1_2_1_2 = new EngineVersion('1.2.1.2');
/**
* Neptune engine version 1.3.0.0
*/
public static readonly V1_3_0_0 = new EngineVersion('1.3.0.0');
/**
* Neptune engine version 1.3.1.0
*/
public static readonly V1_3_1_0 = new EngineVersion('1.3.1.0');
/**
* Neptune engine version 1.3.2.0
*/
public static readonly V1_3_2_0 = new EngineVersion('1.3.2.0');
/**
* Neptune engine version 1.3.2.1
*/
public static readonly V1_3_2_1 = new EngineVersion('1.3.2.1');
/**
* Neptune engine version 1.3.3.0
*/
public static readonly V1_3_3_0 = new EngineVersion('1.3.3.0');
/**
* Neptune engine version 1.3.4.0
*/
public static readonly V1_3_4_0 = new EngineVersion('1.3.4.0');
/**
* Constructor for specifying a custom engine version
* @param version the engine version of Neptune
*/
public constructor(public readonly version: string) { }
}

I think we can simply new EngineVersion('new_version_string'); before we have a separate PR to update the list.

@pahud pahud added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p3 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 14, 2025
@triggan
Copy link
Author

triggan commented Feb 14, 2025

I like the idea of a "catch-all". That seems to be what Aurora are doing here:

/**
* Create a new AuroraMysqlEngineVersion with an arbitrary version.
*
* @param auroraMysqlFullVersion the full version string,
* for example "5.7.mysql_aurora.2.78.3.6"
* @param auroraMysqlMajorVersion the major version of the engine,
* defaults to "5.7"
*/
public static of(auroraMysqlFullVersion: string, auroraMysqlMajorVersion?: string): AuroraMysqlEngineVersion {
return new AuroraMysqlEngineVersion(auroraMysqlFullVersion, auroraMysqlMajorVersion);
}
private static builtIn_5_7(minorVersion: string, addStandardPrefix: boolean = true): AuroraMysqlEngineVersion {
return new AuroraMysqlEngineVersion(`5.7.${addStandardPrefix ? 'mysql_aurora.' : ''}${minorVersion}`);
}
private static builtIn_8_0(minorVersion: string): AuroraMysqlEngineVersion {
// 8.0 of the MySQL engine needs to combine the import and export Roles
return new AuroraMysqlEngineVersion(`8.0.mysql_aurora.${minorVersion}`, '8.0', true);
}
/** The full version string, for example, "5.7.mysql_aurora.1.78.3.6". */
public readonly auroraMysqlFullVersion: string;
/** The major version of the engine. Currently, it's either "5.7", or "8.0". */
public readonly auroraMysqlMajorVersion: string;

However, I am curious as to why we have these enumerated engine versions. This is just a string in our APIs and in CloudFormation. If a user inputs an unsupported engine version, the API throws an Exception. Is the intent here to have better error handling? If so, we have the DescribeDBEngineVersionsCommand API as a means to fetch the current list of supported engine versions. The input can be checked against that list versus relying on commits here to update the list of enumerated versions.

For example, DocDB's L1 construct is just taking a string for engineVersion:

readonly engineVersion?: string;

RDS's EngineVersion class also just accepts an arbitrary string:

export interface EngineVersion {
/**
* The full version string of the engine,
* for example, "5.6.mysql_aurora.1.22.1".
* It can be undefined,
* which means RDS should use whatever version it deems appropriate for the given engine type.
*
* @default - no version specified
*/
readonly fullVersion?: string;
/**
* The major version of the engine,
* for example, "5.6".
* Used in specifying the ParameterGroup family
* and OptionGroup version for this engine.
*/
readonly majorVersion: string;
}

Neptune's L1 construct is auto-generated, so it should be the same as the CFN resource spec for a cluster accepts a string for engine version.

There are also older engine versions in this Neptune L2 construct that are no longer supported (anything older than 1.1.0.0). Unless there's a good reason for having these enumerated versions, I would suggest just removing them and supporting a string for engineVersion.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Feb 14, 2025
@gshpychka
Copy link
Contributor

Unless there's a good reason for having these enumerated versions, I would suggest just removing them and supporting a string for engineVersion.

There is - enums improve DX and that's why they're used pretty widely in AWS CDK.

Reposting from Slack:

Please don't remove enums (this applies to all of CDK). The expectation from the user point of view is that the enums will be updated.

The fact that cloudformation "accepts" any string is really a misnomer, because in reality it doesn't. In the CDK roadmap for 2025 published yesterday, the plan is to add enums for Cloudformation, which improves DX. We shouldn't move in the opposite direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-neptune Related Amazon Neptune effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p3
Projects
None yet
Development

No branches or pull requests

3 participants