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

fix(eks): can't define a cluster with multiple Fargate profiles #8374

Merged
merged 4 commits into from
Jun 10, 2020
Merged

fix(eks): can't define a cluster with multiple Fargate profiles #8374

merged 4 commits into from
Jun 10, 2020

Conversation

njlynch
Copy link
Contributor

@njlynch njlynch commented Jun 4, 2020

Adds a DependsOn Fargate profile resources when more than one Fargate profiles
exists on the same cluster.

fixes #6084


Tested via:

    const vpc = new Vpc(this, 'VPC', {maxAzs: 2});
    const cluster = new FargateCluster(this, 'Cluster', {
      clusterName: 'my-app',
      mastersRole: new Role(this, 'ClusterAdminRole', {
        assumedBy: new AccountRootPrincipal()}
      ),
      vpc,
    });
    const profile1 = cluster.addFargateProfile('MyCustomFargateProfile1', {
      fargateProfileName: 'my-app',
      selectors: [
        {namespace: 'my-app'}
      ],
      vpc
    });
    const profile2 = cluster.addFargateProfile('MyCustomFargateProfile2', {
      fargateProfileName: 'my-app2',
      selectors: [
        {namespace: 'my-app2'}
      ],
      vpc
    });

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

Adds a DependsOn Fargate profile resources when more than one Fargate profiles
exists on the same cluster.

fixes #6084
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: b7a474a
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb
eladb previously requested changes Jun 7, 2020
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather implement this without prepare and also support profiles that are not a direct child of the cluster.

The way I would approach this is by adding an internal method to Cluster (starts with an underscore and has an @internal) annotation) which is called by the FargateProfile class itself during initialization, “registers” the profile on the cluster and perhaps returns the existing profile list (could be a separate api, doesn’t need to be internal).

This will allow each profile to “chain” itself to the last one.

@mergify mergify bot dismissed eladb’s stale review June 8, 2020 11:09

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 3967146
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb
eladb previously approved these changes Jun 9, 2020
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@mergify
Copy link
Contributor

mergify bot commented Jun 9, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot dismissed eladb’s stale review June 9, 2020 09:42

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 2f9df04
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb
eladb previously approved these changes Jun 10, 2020
@mergify mergify bot dismissed eladb’s stale review June 10, 2020 08:43

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 4a1bbfe
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jun 10, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 1e78a68 into aws:master Jun 10, 2020
@njlynch njlynch deleted the njlynch/issue6084 branch June 10, 2020 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[aws-eks] Can't define a fargate cluster with more than one profile
3 participants