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

pkg/ cmd/: kgctl autodetect mesh granularity #197

Merged
merged 1 commit into from
Jun 18, 2021
Merged

Conversation

leonnicolas
Copy link
Collaborator

Addes granularity annotation to auto detect the mesh granularity when
using kubectl

Signed-off-by: leonnicolas leonloechner@gmx.de

@leonnicolas leonnicolas force-pushed the autodetect_granularity branch from 3a1d492 to 0470794 Compare June 18, 2021 11:41
@leonnicolas leonnicolas linked an issue Jun 18, 2021 that may be closed by this pull request
@leonnicolas leonnicolas force-pushed the autodetect_granularity branch 2 times, most recently from 30f4835 to 413a465 Compare June 18, 2021 11:56
// Optain the Granularity by looking at the annotation of the first node.
if opts.granularity == mesh.AutoGranularity {
if len(ns) == 0 {
return errors.New("failed to optain granularity: could not get any nodes")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return errors.New("failed to optain granularity: could not get any nodes")
return errors.New("failed to obtain granularity: could not get any nodes")

@@ -38,6 +39,20 @@ func runGraph(_ *cobra.Command, _ []string) error {
if err != nil {
return fmt.Errorf("failed to list peers: %v", err)
}
// Optain the Granularity by looking at the annotation of the first node.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// Optain the Granularity by looking at the annotation of the first node.
// Obtain the Granularity by looking at the annotation of the first node.

@@ -121,6 +121,19 @@ func runShowConfNode(_ *cobra.Command, args []string) error {
if err != nil {
return fmt.Errorf("failed to list peers: %v", err)
}
// Optain the Granularity by looking at the annotation of the first node.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// Optain the Granularity by looking at the annotation of the first node.
// Obtain the Granularity by looking at the annotation of the first node.

// Optain the Granularity by looking at the annotation of the first node.
if opts.granularity == mesh.AutoGranularity {
if len(ns) == 0 {
return errors.New("failed to optain granularity: could not get any nodes")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return errors.New("failed to optain granularity: could not get any nodes")
return errors.New("failed to obtain granularity: could not get any nodes")

case mesh.LogicalGranularity:
case mesh.FullGranularity:
default:
return fmt.Errorf("mesh granularity %v unsupported", opts.granularity)
Copy link
Owner

@squat squat Jun 18, 2021

Choose a reason for hiding this comment

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

maybe we can make this a more complete phrase?

Suggested change
return fmt.Errorf("mesh granularity %v unsupported", opts.granularity)
return fmt.Errorf("mesh granularity %q is not supported", opts.granularity)

and same above and below.

Also, I think it's nice to quote the values that produce the error to show that it is something funny that came from the user

@@ -208,6 +221,19 @@ func runShowConfPeer(_ *cobra.Command, args []string) error {
if err != nil {
return fmt.Errorf("failed to list peers: %v", err)
}
// Optain the Granularity by looking at the annotation of the first node.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// Optain the Granularity by looking at the annotation of the first node.
// Obtain the Granularity by looking at the annotation of the first node.

// Optain the Granularity by looking at the annotation of the first node.
if opts.granularity == mesh.AutoGranularity {
if len(ns) == 0 {
return errors.New("failed to optain granularity: could not get any nodes")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return errors.New("failed to optain granularity: could not get any nodes")
return errors.New("failed to obtain granularity: could not get any nodes")

@@ -38,6 +39,20 @@ func runGraph(_ *cobra.Command, _ []string) error {
if err != nil {
return fmt.Errorf("failed to list peers: %v", err)
}
// Optain the Granularity by looking at the annotation of the first node.
if opts.granularity == mesh.AutoGranularity {
Copy link
Owner

Choose a reason for hiding this comment

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

How about we turn this block into a helper since we seem to have the same logic repeated 3 times?

Copy link
Owner

@squat squat left a comment

Choose a reason for hiding this comment

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

Nice I like this idea. I think that in the future, we could stick all of these annotations that report a status, vs the ones that are declarative, into a json struct in a kilo.squat.ai/status annotation. I think this could be clearer for the user and would avoid people thinking they can edit the annotations

@leonnicolas leonnicolas force-pushed the autodetect_granularity branch from 413a465 to 4584f01 Compare June 18, 2021 13:58
  Addes granularity annotation to auto detect the mesh granularity when
using kubectl

Signed-off-by: leonnicolas <leonloechner@gmx.de>
@leonnicolas leonnicolas force-pushed the autodetect_granularity branch from 4584f01 to 088578b Compare June 18, 2021 13:59
@squat squat merged commit 9f23e39 into main Jun 18, 2021
@squat squat deleted the autodetect_granularity branch June 18, 2021 14:45
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.

Detect full-mesh automatically for kgctl showconf
2 participants