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

Fargate New cluster #93

Merged
merged 35 commits into from
Oct 18, 2023
Merged

Fargate New cluster #93

merged 35 commits into from
Oct 18, 2023

Conversation

Howlla
Copy link
Contributor

@Howlla Howlla commented Aug 22, 2023

Issue #, if available: 57

Description of changes: New pattern for creating fargate cluster with observability using cloudwatch and containerInsights

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@elamaran11
Copy link
Contributor

@Howlla Please fix GitHub Issues.

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@Howlla Great work, much needed pattern. Have some tactial and closure comments.

  1. Doc is incomplete.
  2. Please merge from main and fix all conflicts and GH issues.

@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-cluster deploy

3 similar comments
@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-cluster deploy

@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-cluster deploy

@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-cluster deploy

lib/common/observability-builder.ts Outdated Show resolved Hide resolved
@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-cluster destroy

2 similar comments
@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-cluster destroy

@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-cluster destroy

@elamaran11
Copy link
Contributor

@Howlla Can you work on the changes pls. I want to merge this and release this week.

@Howlla
Copy link
Contributor Author

Howlla commented Aug 28, 2023

Ready for merge from my side. :)

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@Howlla This is AWSome work, great that you got it working. We had some major breaking change with latest merge that happened yesterday.

  • We are using ObservabilityBuilder from blueprints and removed it from common.
  • Im not sure why you need specific values to two addons, probably because of Fargate. So you need make those changes as optional props in blueprints ObservabilityBuilder and then use those here, which will happen only after next blueprints release.
  • Also the pattern needs to be revamped with latest standards of using enable methods, See other patterns for more details.
  • Also pull from main to push latest code
    Please reachout for any questions.

@elamaran11
Copy link
Contributor

Depdendent on this PR. @Howlla thankyou for your work on this. Please alert me once you have it working, i can take a look,

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

Check the comment

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@Howlla Once the current Release upgrade PR goes, in merge from Main to remove all conflicts. Also please update the doc, remove observabilitybuilder and other extra files noted.

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@Howlla Please follow the guidance for the doc and architecture.

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@Howlla This is AWSome work. Couple of comments on checkin files not related to this work. And Can we raise the bar in deploying a sample workload like yelb app or something like in Java pattern and show some metrics from the App to CW.?

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@Howlla Minor doc feedback. I will run e2e now.

mkdocs.yml Outdated Show resolved Hide resolved
@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-awsnative-fargate-observability deploy

@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-awsnative-fargate-observability destroy

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@Howlla Check this

@elamaran11
Copy link
Contributor

@Howlla Destroy is failing. Please make sure destroy works clean in your environment and fix any issues.

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

LGTM, Nice work

@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-awsnative-fargate-observability deploy

1 similar comment
@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-awsnative-fargate-observability deploy

@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-awsnative-fargate-observability destroy

@elamaran11
Copy link
Contributor

Error is due to CoreDNS failure while destroy. Merging the PR.

@elamaran11 elamaran11 merged commit 4fdc245 into aws-observability:main Oct 18, 2023
2 checks passed
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.

2 participants