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

feat(helm:daemonsets): parametrize resources configuration using values #63

Merged
merged 1 commit into from
Jul 18, 2024
Merged

feat(helm:daemonsets): parametrize resources configuration using values #63

merged 1 commit into from
Jul 18, 2024

Conversation

m00lecule
Copy link
Contributor

@m00lecule m00lecule commented Jul 16, 2024

resolves:
#62
#61

Introduced resources fields to values.yaml and replaced hardcoded daemonsets resources. Hardcoded resources prevented users from capacity customization.

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

@m00lecule
Copy link
Contributor Author

@lisguo may I ask for a review?

@lisguo
Copy link
Contributor

lisguo commented Jul 17, 2024

We don't have a good mechanism for running the checks from forks of the repo. Let me push your changes to a branch just to test that this passes the tests.

@lisguo
Copy link
Contributor

lisguo commented Jul 17, 2024

@m00lecule
Copy link
Contributor Author

@lisguo I see those pipelines are failing on AWS Credentials step, which is not looking like an issue related to helm. Could You help me figure this out?

@lisguo
Copy link
Contributor

lisguo commented Jul 18, 2024

@lisguo I see those pipelines are failing on AWS Credentials step, which is not looking like an issue related to helm. Could You help me figure this out?

Yeah this is an issue if you make a PR from a fork instead of the same branch, so it will not have the same creds that we run on our side for integration tests. I ended up pulling in your branch into the repo and saw that the tests pass per the link above.

LGTM.

@lisguo lisguo merged commit 41c42db into aws-observability:main Jul 18, 2024
3 of 6 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.

4 participants