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

Added cni-metrics-helper docs #2187

Merged
merged 4 commits into from
Dec 28, 2022
Merged

Added cni-metrics-helper docs #2187

merged 4 commits into from
Dec 28, 2022

Conversation

0xquark
Copy link
Contributor

@0xquark 0xquark commented Dec 28, 2022

What type of PR is this?
This PR adds README.md to cni-metrics-helper as part of the docs

Which issue does this PR fix:
#2143

What does this PR do / Why do we need it:
Ease of use and better customer experience.

If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:

Testing done on this change:

Automation added to e2e:

Will this PR introduce any new dependencies?:

Will this break upgrades or downgrades. Has updating a running cluster been tested?:

Does this change require updates to the CNI daemonset config files to work?:

Does this PR introduce any user-facing change?:

Added resources field to cni-metrics-helper chart

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

@0xquark 0xquark requested a review from a team as a code owner December 28, 2022 14:49
@0xquark
Copy link
Contributor Author

0xquark commented Dec 28, 2022

Hi @jdn5126 ! Thanks for the help. I've made a PR on this issue. Few things i want you to take a look is :

  1. I've used this git clone way to install the cni-metrics-helper. We can do the same with helm repo add amazon-vpc-cni-k8s https://aws.github.io/amazon-vpc-cni-k8s/ and then using helm install. But this gave me an error of chart not existing.So the above method i used to install cni-metrics-helper is fine?
  2. I've added the Resources section in the README.md , is that what you meant by it? or do i need to add a column for resources in the values.yaml file? ( I've used the default values for the resources as available on internet)
  3. I've added all the parameters in the readme which were present in values.yaml file.
    Let me know if there are any more parameters to add or any way i can improve the documentation.

Copy link
Contributor

@jdn5126 jdn5126 left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this! Just a few nits and then this is good to go

@jdn5126
Copy link
Contributor

jdn5126 commented Dec 28, 2022

Oh sorry, I missed these questions before reviewing:

  1. Yeah, the method you used is fine. Eventually, this chart will be published at https://github.com/aws/eks-charts , and that will be the recommended helm repo add.
  2. So we need the resources section to be present in the values.yaml file for this to work properly. I added some comments in the review, but feel free to reach out if you have any questions.
  3. The documentation looks excellent, thanks again!

Increment helm chart version to 0.1.13

Modified to include resources from values.yaml
@jdn5126 jdn5126 merged commit a5823c0 into aws:master Dec 28, 2022
@jdn5126
Copy link
Contributor

jdn5126 commented Dec 28, 2022

Merged, thanks again @0xquark !

@0xquark
Copy link
Contributor Author

0xquark commented Dec 28, 2022

Merged, thanks again @0xquark !

Thank you so much @jdn5126 for helping out! It was greatly appreciated.
Looking forward to contributing more in future! 🚀 🚀 🚀

jdn5126 added a commit to jdn5126/amazon-vpc-cni-k8s that referenced this pull request Feb 2, 2023
jdn5126 added a commit to jdn5126/amazon-vpc-cni-k8s that referenced this pull request Feb 2, 2023
jdn5126 added a commit to jdn5126/amazon-vpc-cni-k8s that referenced this pull request Feb 3, 2023
jdn5126 added a commit to jdn5126/amazon-vpc-cni-k8s that referenced this pull request Feb 3, 2023
jdn5126 added a commit to jdn5126/amazon-vpc-cni-k8s that referenced this pull request Feb 7, 2023
jdn5126 added a commit to jdn5126/amazon-vpc-cni-k8s that referenced this pull request Feb 7, 2023
jdn5126 added a commit to jdn5126/amazon-vpc-cni-k8s that referenced this pull request Feb 7, 2023
jdn5126 added a commit that referenced this pull request Feb 7, 2023
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