cdk-eks-blueprints icon indicating copy to clipboard operation
cdk-eks-blueprints copied to clipboard

bugfix: Adds cluster role for otel daemonset deployment mode

Open arunvthangaraj opened this issue 1 year ago • 5 comments

Issue #, if available: 990

Description of changes: This PR adds the required cluster role for the daemonset template.

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

arunvthangaraj avatar Apr 23 '24 22:04 arunvthangaraj

@shapirov103 Please approve and Merge. E2E is not required.

elamaran11 avatar Apr 23 '24 22:04 elamaran11

/do-e2e-tests

shapirov103 avatar Apr 25 '24 19:04 shapirov103

/do-e2e-tests

shapirov103 avatar Apr 25 '24 19:04 shapirov103

@shapirov103 Please approve and Merge. E2E is not required.

@elamaran11 aren't we deploying this addon in e2e, so if there is an error in the modified kubernetes manifest we should be able to catch it. Ideally, functional verification is also needed, e.g. similar to what we do in Conformitron. That will improve quality assurance and reduce issues from customers.

I will look into the e2e failure.

shapirov103 avatar May 01 '24 03:05 shapirov103

@shapirov103 Please approve and Merge. E2E is not required.

@elamaran11 aren't we deploying this addon in e2e, so if there is an error in the modified kubernetes manifest we should be able to catch it. Ideally, functional verification is also needed, e.g. similar to what we do in Conformitron. That will improve quality assurance and reduce issues from customers.

I will look into the e2e failure.

@shapirov103 We do have E2E for this addon already but the addon supports 4 different types of modes - deployment daemonset statefulset and sidecar we cant accomodate all of these in one e2e deployment. So our e2e deploys only deployment mode. We can alter it but we dont use daemonset approach in most situations as its an anti-pattern. Arun is working on a blog with daemonset so this was broken and this change is required. But e2e failure is not related to this change. So once e2e issues are fixed, we can merge this.

elamaran11 avatar May 01 '24 12:05 elamaran11

@shapirov103 Can this be merged?

elamaran11 avatar May 17 '24 14:05 elamaran11

Yes, the error on e2e was unrelated.

shapirov103 avatar May 17 '24 14:05 shapirov103