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

helm: Expose l2 neigh discovery related agent flags #17526

Merged
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
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.5 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
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
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
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.5 Oct 7, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.5 Oct 7, 2021
nbusseneau added a commit to nbusseneau/cilium that referenced this pull request Oct 11, 2021
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>
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.5 Oct 12, 2021
errordeveloper pushed a commit that referenced this pull request Oct 12, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
No open projects
1.10.5
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

6 participants