Skip to content

Document that clustermesh cluster-id range is 1-255 #19683

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
May 10, 2022
Merged

Document that clustermesh cluster-id range is 1-255 #19683

merged 1 commit into from
May 10, 2022

Conversation

stonith
Copy link
Contributor

@stonith stonith commented May 4, 2022 •
edited
Loading

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

The documented clustermesh cluster-id range is 0-255 but actually seems to be 1-255 as per https://github.com/cilium/cilium/blob/master/daemon/cmd/daemon.go#L1188-L1190

@stonith stonith requested review from a team as code owners May 4, 2022 04:29
@stonith stonith requested review from jrajahalme and joestringer May 4, 2022 04:29
@maintainer-s-little-helper
Copy link

Commit 590476e99c9c410c535af787de622647c4d5ff5f does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 4, 2022
@stonith
Copy link
Contributor Author

stonith commented May 4, 2022

Signed-off-by: Darren Foo stonith@users.noreply.github.com

@joestringer joestringer changed the title clustermesh cluster-id range is 1-255 Document that clustermesh cluster-id range is 1-255 May 4, 2022
@joestringer joestringer added release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.11 labels May 4, 2022
@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 May 4, 2022
joestringer
joestringer approved these changes May 4, 2022
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. The Signed-off line needs to be inside the commit message, which you can do using git commit --amend and by force-pushing the updated commit into your branch here.

@stonith stonith force-pushed the update_cluster-id_range branch from 590476e to 1f5627a Compare May 4, 2022 18:05
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 4, 2022
sayboras
sayboras approved these changes May 6, 2022
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks

@kaworu kaworu added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label May 6, 2022
kaworu
kaworu approved these changes May 6, 2022
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.

Thanks!

@sayboras sayboras added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 9, 2022
@sayboras
Copy link
Member

sayboras commented May 9, 2022

can you help to sign off the commit as per Joe's comment to make CI happy ? Thanks

https://github.com/cilium/cilium/runs/6321874286?check_suite_focus=true

@joestringer
Copy link
Member

joestringer commented May 9, 2022 •
edited
Loading

It looks like the script is not picking up the signed-off-by line because the formatting is unusual. Note that you can use git commit --amend --signoff to get the appropriate format (assuming you already configured your git settings). In particular, the email is expected to be within angle brackets, ie John Doe <john@doe.com>.

@stonith
clustermesh cluster-id range is 1-255
4a31aaf 
Signed-off-by: Darren Foo <stonith@users.noreply.github.com>
@stonith stonith force-pushed the update_cluster-id_range branch from 1f5627a to 4a31aaf Compare May 9, 2022 18:02
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 9, 2022
jrajahalme
jrajahalme approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

I'm not exactly sure where the non-zero limitation comes from, but as cilium-cli warns for it we might as well document it like proposed here. Btw. cilium-cli also warns if a cluster name is default, but clustermesh might still work as cluster name should not really mattar that much.

@kaworu
Copy link
Member

kaworu commented May 10, 2022

This is a pure documentation change, no need for CI to run. All reviews are in, marking as ready-to-merge.

@kaworu kaworu added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 10, 2022
@kaworu kaworu merged commit 81b1a62 into cilium:master May 10, 2022
@christarazi christarazi mentioned this pull request May 17, 2022
Merged
@jibi jibi added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels May 23, 2022
@joestringer joestringer mentioned this pull request Jun 13, 2022
Merged
@aanm aanm mentioned this pull request Jun 22, 2022
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

@joestringer joestringer joestringer approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. 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.
7 participants
@stonith @sayboras @joestringer @kaworu @jrajahalme @jibi @christarazi