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

Add Documentation #18

Merged
merged 1 commit into from
May 9, 2023
Merged

Add Documentation #18

merged 1 commit into from
May 9, 2023

Conversation

adrianchiris
Copy link
Collaborator

  • Update README
  • Add CONTRIBUTING doc

@adrianchiris adrianchiris changed the title Add Documentation [DNM] Add Documentation May 9, 2023
@coveralls
Copy link

coveralls commented May 9, 2023

Coverage Status

Coverage: 76.366%. Remained the same when pulling e12adbf on adrianchiris:update-readme into a1e2c92 on Mellanox:main.

README.md Outdated Show resolved Hide resolved
@adrianchiris adrianchiris changed the title [DNM] Add Documentation Add Documentation May 9, 2023
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -23,20 +165,101 @@ data:
config: |
{
"pools": {
"pool1": {"subnet": "192.168.0.0/16", "perNodeBlockSize": 100 , "gateway": "192.168.0.1"},
"pool2": {"subnet": "172.16.0.0/16", "perNodeBlockSize": 50 , "gateway": "172.16.0.1"}
"my-pool": {"subnet": "192.168.0.0/16", "perNodeBlockSize": 100 , "gateway": "192.168.0.1"},
Copy link
Contributor

Choose a reason for hiding this comment

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

remove last , so it will be valid json

README.md Outdated Show resolved Hide resolved
README.md Outdated
The address the probe endpoint binds to. (default ":8081")
--kubeconfig string
Paths to a kubeconfig. Only required if out-of-cluster.
--leader-elect
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we don't support it. Maybe we should document only the things that will work

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the code it is supported.
I added [1] extra config option to configure namespace for leader election process. Also enabled leader election by default to protect from the situation when a user decided to manually scale the deployment.

[1] #19

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are the supported flags by the binary, should i remove or keep ?

either way #19 would need to update this file as well once we merge it.

Copy link
Collaborator Author

@adrianchiris adrianchiris May 9, 2023

Choose a reason for hiding this comment

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

removing leader elect flag for now

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@adrianchiris
Copy link
Collaborator Author

@ykulazhenkov @moshe010 PTAL

lets get this one in

README.md Outdated Show resolved Hide resolved
- Update README
- Add CONTRIBUTING doc

Signed-off-by: adrianc <adrianc@nvidia.com>

## Contributing

We welcome your feedback and contributions to this project. Please see the [CONTRIBUTING.md](https://github.com/Mellanox/nvidia-k8s-ipam/blob/main/CONTRIBUTING.md)
Copy link
Collaborator

@ykulazhenkov ykulazhenkov May 9, 2023

Choose a reason for hiding this comment

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

nit: Do we want to use relative link here? [CONTRIBUTING.md](CONTRIBUTING.md)

Copy link
Contributor

Choose a reason for hiding this comment

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

we will address it in follow-up

@moshe010 moshe010 merged commit 87ee551 into Mellanox:main May 9, 2023
8 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.

None yet

4 participants