Skip to content

helm: Expose l2 neigh discovery related agent flags #17526

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 7, 2021
Merged

helm: Expose l2 neigh discovery related agent flags #17526

merged 1 commit into from
Oct 7, 2021

Conversation

brb
Copy link
Member

@brb brb commented Oct 4, 2021

This commit exposes the following flags:

  • --enable-l2-neigh-discovery via l2NeighDiscovery.enabled.
  • --arping-refresh-period via l2NeighDiscovery.refreshPeriod.

@brb brb added area/helm Impacts helm charts and user deployment experience needs-backport/1.10 labels Oct 4, 2021
@brb brb requested a review from a team as a code owner October 4, 2021 13:59
@brb brb requested review from a team, kaworu and nathanjsweet October 4, 2021 13:59
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 4, 2021
@brb brb added the release-note/misc This PR makes changes that have no direct user impact. label Oct 4, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 4, 2021
@brb brb force-pushed the pr/brb/helm-enable-l2-neigh-discovery branch from 200aeae to b97b9cc Compare October 4, 2021 14:34
kaworu
kaworu requested changes Oct 5, 2021
View reviewed changes
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Both values should be added to install/kubernetes/cilium/values.yaml with a leading comment starting with --.

Then, run make -C install/kubernetes docs in order to update install/kubernetes/cilium/README.md.

@brb brb force-pushed the pr/brb/helm-enable-l2-neigh-discovery branch from b97b9cc to b589cda Compare October 5, 2021 09:41
@brb brb requested a review from kaworu October 5, 2021 09:41
@brb
helm: Expose l2 neigh discovery related agent flags
b589cda 
This commit exposes the following flags:
- "--enable-l2-neigh-discovery" via "l2NeighDiscovery.enabled".
- "--arping-refresh-period" via "l2NeighDiscovery.refreshPeriod".

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Member Author

brb commented Oct 6, 2021

test-1.20-4.19

@brb brb added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 6, 2021
@errordeveloper errordeveloper merged commit da74607 into master Oct 7, 2021
@errordeveloper errordeveloper deleted the pr/brb/helm-enable-l2-neigh-discovery branch October 7, 2021 15:35
@christarazi christarazi mentioned this pull request Oct 7, 2021
Merged
@nbusseneau nbusseneau mentioned this pull request Oct 11, 2021
Merged
nbusseneau added a commit to nbusseneau/cilium that referenced this pull request Oct 11, 2021
@nbusseneau
docs: fix docs following cilium#17526
4b9b9a1 
It seems we forgot to update `helm-values.rst` in cilium#17526, yielding
issues when building documentation locally with `make render-docs`:

```
Please fix the following spelling mistakes:
* Documentation/helm-reference.rst:876: (NeighDiscovery)
* Documentation/helm-reference.rst:876: (arping)
* Documentation/helm-reference.rst:877: (arping)
* Documentation/helm-reference.rst:880: (NeighDiscovery)
* Documentation/helm-reference.rst:881: (neighbour)
* Documentation/helm-values.rst:876: (NeighDiscovery)
* Documentation/helm-values.rst:876: (arping)
* Documentation/helm-values.rst:877: (arping)
* Documentation/helm-values.rst:880: (NeighDiscovery)
* Documentation/helm-values.rst:881: (neighbour)

If the words are not misspelled, run:
Documentation/update-spelling_wordlist.sh NeighDiscovery arping neighbour
```

Since we already have more occurrences of `neighbor` (US spelling) in
the repo, we replace `neighbour` with `neighbour` for consistency and
then fix `helm-values.rst` + spelling wordlist.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
errordeveloper pushed a commit that referenced this pull request Oct 12, 2021
@nbusseneau @errordeveloper
docs: fix docs following #17526
55e7fbb 
It seems we forgot to update `helm-values.rst` in #17526, yielding
issues when building documentation locally with `make render-docs`:

```
Please fix the following spelling mistakes:
* Documentation/helm-reference.rst:876: (NeighDiscovery)
* Documentation/helm-reference.rst:876: (arping)
* Documentation/helm-reference.rst:877: (arping)
* Documentation/helm-reference.rst:880: (NeighDiscovery)
* Documentation/helm-reference.rst:881: (neighbour)
* Documentation/helm-values.rst:876: (NeighDiscovery)
* Documentation/helm-values.rst:876: (arping)
* Documentation/helm-values.rst:877: (arping)
* Documentation/helm-values.rst:880: (NeighDiscovery)
* Documentation/helm-values.rst:881: (neighbour)

If the words are not misspelled, run:
Documentation/update-spelling_wordlist.sh NeighDiscovery arping neighbour
```

Since we already have more occurrences of `neighbor` (US spelling) in
the repo, we replace `neighbour` with `neighbour` for consistency and
then fix `helm-values.rst` + spelling wordlist.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@joestringer joestringer mentioned this pull request Oct 13, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaworu kaworu kaworu approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@brb @kaworu @nathanjsweet @joestringer @errordeveloper @christarazi