-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(stepfunctions-tasks): adding on demand option + allocation s… #12512
Conversation
…trategy for emrcreatecluster step The current InstanceFleetProvisioningSpecificationsProperty (https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-stepfunctions-tasks.EmrCreateCluster.InstanceFleetProvisioningSpecificationsProperty.html) only supports spot specification whereas the EMR InstanceFleetProvisioningSpecifications supports OnDemandSpecification (https://docs.aws.amazon.com/emr/latest/APIReference/API_InstanceFleetProvisioningSpecifications.html#EMR-Type-InstanceFleetProvisioningSpecifications-OnDemandSpecification). This commit adds the missing options. fixes aws#12501
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts
Outdated
Show resolved
Hide resolved
* | ||
* @experimental | ||
*/ | ||
export interface OnDemandProvisioningSpecificationProperty { |
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.
naming, we generally use Props
as a suffix for constructs and Options
as a suffix otherwise.
maybe this should be called OnDemandProvisioningOptions
or something simpler.
It was a miss and something we should clean up when we drop the experimental
for the already existing SpotProvisioningSpecificationProperty
interface. But let's not proliferate it further 🙂
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.
I have followed existing conventions of using Property throughout. I think changing this to Props would require changing existing names to Props as well and that would introduce a breaking change.
As for the experimental annotation, its used all over this file, is it fine to remove them all?
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.
sure, let's leave it alone for now. i'm not a fan of proliferating something we know we want to change because that's the way it's always been done.
but i'm happy to do it in another change. it will break more things, but that's the only way to stop the trend.
/** | ||
* Render the SpotProvisioningSpecificationProperty to JSON | ||
* | ||
* @param property | ||
*/ | ||
export function SpotProvisioningSpecificationPropertyToJson(property: EmrCreateCluster.SpotProvisioningSpecificationProperty) { | ||
return { | ||
AllocationStrategy: cdk.stringToCloudFormation(property.allocationStrategy), | ||
BlockDurationMinutes: cdk.numberToCloudFormation(property.blockDurationMinutes), | ||
TimeoutAction: cdk.stringToCloudFormation(property.timeoutAction?.valueOf()), | ||
TimeoutDurationMinutes: cdk.numberToCloudFormation(property.timeoutDurationMinutes), | ||
}; | ||
} |
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.
this is only used in one location from what I can tell, does it need to be in a shared utils file?
let's avoid future proofing until the need arises.
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.
I could see all the conversion functions are in the utils file. Do you have a suggestion on how we can break them up?
/** | ||
* Render the SpotProvisioningSpecificationProperty to JSON | ||
* | ||
* @param property | ||
*/ | ||
export function OnDemandProvisioningSpecificationPropertyToJson(property: EmrCreateCluster.OnDemandProvisioningSpecificationProperty) { | ||
return { | ||
AllocationStrategy: cdk.stringToCloudFormation(property.allocationStrategy), | ||
}; | ||
} | ||
|
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.
does this need to be exported? it's only used locally within this file
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.
I have also followed convention here all the functions in cluster-utils.ts
are exported.
SpotSpecification: | ||
property.spotSpecification === undefined | ||
? property.spotSpecification | ||
: SpotProvisioningSpecificationPropertyToJson(property.spotSpecification), |
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.
SpotSpecification: | |
property.spotSpecification === undefined | |
? property.spotSpecification | |
: SpotProvisioningSpecificationPropertyToJson(property.spotSpecification), | |
SpotSpecification: property?.spotSpecification ?? SpotProvisioningSpecificationPropertyToJson(property.spotSpecification, |
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.
This would be wrong, we don't want to convert to JSON if the field is undefined.
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.
shouldn't the function handle that?
OnDemandSpecification: | ||
property.onDemandSpecification === undefined | ||
? property.onDemandSpecification | ||
: OnDemandProvisioningSpecificationPropertyToJson(property.onDemandSpecification), |
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.
OnDemandSpecification: | |
property.onDemandSpecification === undefined | |
? property.onDemandSpecification | |
: OnDemandProvisioningSpecificationPropertyToJson(property.onDemandSpecification), | |
OnDemandSpecification: property?.onDemandSpecification ?? OnDemandProvisioningSpecificationPropertyToJson(property.onDemandSpecification), |
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.
Same as above.
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.
same question about the function. can it be more robust to return undefined if the property is undefined?
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts
Outdated
Show resolved
Hide resolved
} | ||
|
||
|
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.
nit: remove extra line
…ws#12508) Bumps [typescript-json-schema](https://github.com/YousefED/typescript-json-schema) from 0.46.0 to 0.47.0. - [Release notes](https://github.com/YousefED/typescript-json-schema/releases) - [Commits](YousefED/typescript-json-schema@v0.46.0...v0.47.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
…cts (aws#12504) Introduce an eslint rule & fix all instances in our code which refer to `@aws-cdk/core.Construct` using a qualified namespace (e.g. `core.Construct`, `cdk.Construct`, etc). In v2, `Construct` will refer to `constructs.Construct` so it will reduce the chances for merge issues. The rule fixer automatically adds an `import` statement (separated from the main import group) and replaces the usage. If the file already imports `constructs.Construct`, then the new import will alias as `CoreConstruct`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Bumps [aws-sdk](https://github.com/aws/aws-sdk-js) from 2.827.0 to 2.828.0. - [Release notes](https://github.com/aws/aws-sdk-js/releases) - [Changelog](https://github.com/aws/aws-sdk-js/blob/master/CHANGELOG.md) - [Commits](aws/aws-sdk-js@v2.827.0...v2.828.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
…ion (aws#12509) The script currently does not support publishing CDKv2. Update the script so as to read the baseline version by resolving the version from the repo's current state (from `resolve-version.js`). If the resolved version is not present in NPM, usually because the version is not *yet* published, fallback to an environment variable `NPM_DISTTAG` that specifies the dist tag to use. ref: aws/cdk-ops#1151 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…aws#12272) Closes aws#12256 Clarified the usage of custom user data with default or custom AMI by adding another example and pointing out the differences. Changed the launch template reference in the examples to use the latest version rather than the default. If you don't know the difference it would be unexpected for the user to change the user data without having it applied to the nodes after deploy. Removed unnecessary wrapping of instance type string with typed version and calling toString. Make the sentence about defining instance type either in launch template or node group more understandable. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
We were missing a few cases where we did not validate that a package had the correct `maturity` set: - A package with L2s could still have a `maturity` of 'cfn-only' - A package with only L1s could still have a non-'cfn-only' `maturity` Fix that in the linter, along with the violations discovered. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
We've moved off the npm client and onto yarn a long time ago. These scripts are still using the npm client. Instead switch them to use the `yarn` command. No substantial difference, just consistency. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws#12527) Oversight, the README currently refers to the [lambda-layer-awscli](https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/lambda-layer-awscli) module. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
ElasticSearch-support for UltraWarm nodes including validations for necessary pre-requirements. Closes aws#6462. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Bumps [esbuild](https://github.com/evanw/esbuild) from 0.8.32 to 0.8.33. - [Release notes](https://github.com/evanw/esbuild/releases) - [Changelog](https://github.com/evanw/esbuild/blob/master/CHANGELOG.md) - [Commits](evanw/esbuild@v0.8.32...v0.8.33) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
…3.0 (aws#12561) Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 4.12.0 to 4.13.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v4.13.0/packages/eslint-plugin) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Many people don't commit or want to commit `cdk.context.json`, and are subsequently annoyed that they can't use `Vpc.fromLookup()` in a pipeline. Make it very clear that this is the way CDK is designed to work and they must do it. Relates to aws#12160 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 10.17.48 to 10.17.51. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Closes aws#12381 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws#11479) (aws#12549) closes aws#11479 ---- Hello, picking this up as my first issue to learn about how CDK works: [AWS mentions](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html) that iam groups can't be principals in policies. If a developer doesn't know this, they get an error during deployment. This change will throw an error to help them catch the issue during synthesis time. Note that this doesn't handle the code path where an arn string is specified as the principal, but the method already documents that a group would be invalid. Let me know if there's a cleaner way to fix this.
) Bumps [@types/lodash](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/lodash) from 4.14.167 to 4.14.168. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/lodash) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
…4.0 (aws#12581) Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 4.13.0 to 4.14.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v4.14.0/packages/eslint-plugin) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
…ric (aws#12455) Adds the missing predefined metric for MSK storage auto scaling. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…aws#12558) Add `codeBuildCloneOutput` property to the CodeCommit source action. It automatically adds the `codecommit:GetRepository` permission to the CodeCommitSourceAction role. It will also add the `codecommit:GitPull` permission to any CodeBuildAction using the artifact from CodeCommitSourceAction as input. Closes aws#12236 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Bumps [esbuild](https://github.com/evanw/esbuild) from 0.8.33 to 0.8.34. - [Release notes](https://github.com/evanw/esbuild/releases) - [Changelog](https://github.com/evanw/esbuild/blob/master/CHANGELOG.md) - [Commits](evanw/esbuild@v0.8.33...v0.8.34) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
`Data` -> `JsonPath` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…instead of ARN (aws#12436) feat(stepfunctions-tasks): EcsRunTask now uses taskDefinition family instead of ARN Currently the ECS run task implementation uses full ARN of the task definition. This ARN contains the ACTIVE revision at the end. The ACTIVE revision keeps on changing as the task definition changes causing potential failures (refer the issue). This change now lets the run task API to use task definition family (and corresponding ARN which does not contain the revision) to run the task. Using the family would mean that the latest ACTIVE revision of task-definition is used always. This is supported out of the box by ECS (refer the below refs). Parameter Ref: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_RunTask.html#ECS-RunTask-request-taskDefinition Permissions Ref: https://docs.aws.amazon.com/step-functions/latest/dg/ecs-iam.html closes aws#12080 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
In aws#12394, the ability was added to read context from the CDK config file in the user home directory. However, this interferes with the unit tests which expect a pristine config, so the unit tests fail on a machine with a `~/.cdk.json` file that has context. Fix that. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ute (aws#12528) Currently when we define any HTTP integration (like ALB, NLB etc.) explicitly and use it for multiple routes, the `HttpRoute` construct will create a new `HttpIntegration` resource for each of these routes. [Ref](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-apigatewayv2/lib/http/route.ts#L128) These extra HttpIntegration (and hence `CfnIntegration`) are not required since the same integration can be used for all the routes. This feature is supported in console as well (i.e. selecting same integration for multiple routes). A sample snippet: ```ts const apiGateway = new apigw.HttpApi(this, apiLableApiGateway); const apiIntegration = new apigwint.HttpAlbIntegration({ listener : apiListener, vpcLink : apiVpcLink }); const apiRoutes = [ { method: apigw.HttpMethod.ANY, path: '/' }, { method: apigw.HttpMethod.GET, path: '/status' }, { method: apigw.HttpMethod.GET, path: '/whateverelse' }, ]; for (const apiRouteIndex in apiRoutes) { const apiRouteMethod = apiRoutes[apiRouteIndex].method; const apiRoutePath = apiRoutes[apiRouteIndex].path; apiGateway.addRoutes({ methods : [apiRouteMethod], path : apiRoutePath, integration : apiIntegration, }); } ``` The root cause of this issue is that the current api resource does not keep track of any existing integrations with the same integration config. We are solving this issue by keeping track of these integrations in an internal map with key as stringified value of `HttpRouteIntegrationConfig` (which we get by calling the `bind` function) and value as the `HttpIntegration`. This ensures that we reuse any existing integration (with same config) instead of creating a new one for every route. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…12652) This is in order to minimize the merge conflicts with the `v2-main` branch, where we will deprecate the `@aws-cdk/assets` module. Unfortunately, we can't deprecate this module on the main V1 branch, as the [`CopyOptions` struct](https://github.com/aws/aws-cdk/blob/d169688f35bc78c88c44ff9a7d8fa0dfea71f904/packages/%40aws-cdk/assets/lib/fs/options.ts#L8-L14) inside of it is incompatible with the non-deprecated version in `@aws-cdk/core`, [also called `CopyOptions`](https://github.com/aws/aws-cdk/blob/d169688f35bc78c88c44ff9a7d8fa0dfea71f904/packages/%40aws-cdk/core/lib/fs/options.ts#L64-L70) - the `follow` property has different types in the two modules, and the `CopyOptions` in `@aws-cdk/assets` is widely used (through interface inheritance) in the Construct Library by many stable modules like Lambda, ECS, EC2, and CodeBuild. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ags (aws#12645) CDKv1 introduced feature flags that allowed users to opt-in to new backwards-incompatible behaviour. There are two types of flags - one set that need to be entirely removed in CDKv2 and their 'enabled' behaviour made the default, and another set where the the new behaviour should be made the default, and allow users to explicitly opt-out. This change addresses the testing strategy for the former set (and not the latter). There exist unit tests that test the behaviour both when the feature flags are enabled and feature flags and disabled. However, in v2, the feature flags will be removed and its usage blocked. The default behaviour will be as if the feature flag is enabled. Introduce branching logic that treats these unit tests depending on whether it's executed in the context of CDKv1 or CDKv2. The logic is as follows: - In the context of v1, all tests will execute as normal. - In the context of v2, tests that rely on feature flag to be unset will not be executed. - In the context of v2, tests that rely on feature flag to be set will not configure the feature flag on the App's context and continue to execute the test. This logic has been captured at a single location in two methods - `testFeatureFlag()` and `testFeatureFlagDisabled()`. To validate this logic, the unit tests that rely on the feature flags `aws-kms:defaultKeyPolicies`, `aws-secretsmanager:parseOwnedSecretName` and `core:enableStackNameDuplicates` have been updated to use these methods. Forward looking: The final PR in v2 is expected to look like this - aws#12644 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…trategy for emrcreatecluster step The current InstanceFleetProvisioningSpecificationsProperty (https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-stepfunctions-tasks.EmrCreateCluster.InstanceFleetProvisioningSpecificationsProperty.html) only supports spot specification whereas the EMR InstanceFleetProvisioningSpecifications supports OnDemandSpecification (https://docs.aws.amazon.com/emr/latest/APIReference/API_InstanceFleetProvisioningSpecifications.html#EMR-Type-InstanceFleetProvisioningSpecifications-OnDemandSpecification). This commit adds the missing options. fixes aws#12501
Pull request has been modified.
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.
I also think the integration test needs to be updated / an integration test needs to be written (either/or).
/** | ||
* Create a new AllocationStrategy with an arbitrary allocation strategy. | ||
* | ||
* @param customAllocationStrategy the allocation strategy string, | ||
* for example "lowest-price" | ||
*/ |
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.
are there any docs links we can also point to?
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.
Added link to the Cfn docs.
* Different allocation strategies that can be used by the EMR cluster | ||
*/ | ||
export class AllocationStrategy { | ||
/** lowest-price allocation strategy. */ |
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.
include a link to the docs - ideally we want to provide more information to users to determine which allocation is most appropriate for their use case(s)
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.
Done, added link to the Cfn docs.
* | ||
* @experimental | ||
*/ | ||
export interface OnDemandProvisioningSpecificationProperty { |
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.
sure, let's leave it alone for now. i'm not a fan of proliferating something we know we want to change because that's the way it's always been done.
but i'm happy to do it in another change. it will break more things, but that's the only way to stop the trend.
SpotSpecification: | ||
property.spotSpecification === undefined | ||
? property.spotSpecification | ||
: SpotProvisioningSpecificationPropertyToJson(property.spotSpecification), |
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.
shouldn't the function handle that?
OnDemandSpecification: | ||
property.onDemandSpecification === undefined | ||
? property.onDemandSpecification | ||
: OnDemandProvisioningSpecificationPropertyToJson(property.onDemandSpecification), |
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.
same question about the function. can it be more robust to return undefined if the property is undefined?
/** | ||
* Render the SpotProvisioningSpecificationProperty to JSON | ||
* | ||
* @param property | ||
*/ | ||
export function SpotProvisioningSpecificationPropertyToJson(property: EmrCreateCluster.SpotProvisioningSpecificationProperty) { | ||
return { | ||
AllocationStrategy: cdk.stringToCloudFormation(property.allocationStrategy?.name), | ||
BlockDurationMinutes: cdk.numberToCloudFormation(property.blockDurationMinutes), | ||
TimeoutAction: cdk.stringToCloudFormation(property.timeoutAction?.valueOf()), | ||
TimeoutDurationMinutes: cdk.numberToCloudFormation(property.timeoutDurationMinutes), | ||
}; | ||
} | ||
|
||
/** | ||
* Render the SpotProvisioningSpecificationProperty to JSON | ||
* | ||
* @param property | ||
*/ | ||
export function OnDemandProvisioningSpecificationPropertyToJson(property: EmrCreateCluster.OnDemandProvisioningSpecificationProperty) { | ||
return { | ||
AllocationStrategy: cdk.stringToCloudFormation(property.allocationStrategy?.name), | ||
}; | ||
} | ||
|
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.
why do both of these need to be in a utils file? - they're only used in one file. the preference is to keep it local until there's a need (multiple usage sites) to move it to a common location.
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@mohanrajendran are you still working through this PR? anything I can help with to get it going again? |
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.
@mohanrajendran looking close, but there are still a few unaddressed comments. is this still being worked on?
Is this being actively developed? This a feature that would be a big win for our current Step Function and make migrating to flexible fleets incredibly easy for our team. Otherwise, we will need to re-haul our step function. |
@zanussbaum It looks like development on this feature has stalled – please feel free to pick up the work if you are interested! |
924c117
to
ebfd5f2
Compare
This PR has been deemed to be abandoned, and will be closed. Please create a new PR for these changes if you think this decision has been made in error. |
…trategy for emrcreatecluster step
The current InstanceFleetProvisioningSpecificationsProperty (https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-stepfunctions-tasks.EmrCreateCluster.InstanceFleetProvisioningSpecificationsProperty.html) only supports spot specification whereas the EMR InstanceFleetProvisioningSpecifications supports OnDemandSpecification (https://docs.aws.amazon.com/emr/latest/APIReference/API_InstanceFleetProvisioningSpecifications.html#EMR-Type-InstanceFleetProvisioningSpecifications-OnDemandSpecification). This commit adds the missing options.
fixes #12501
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license