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 new graph setting for the animation style #851

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jshaughn
Copy link
Contributor

@jshaughn jshaughn added the test: n/a PR does not need test additions or updates label Nov 22, 2024
@jshaughn jshaughn self-assigned this Nov 22, 2024
Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

You forgot to add the default to the example CR, and I just noticed we are missing all of the settings here. So you should add them. This example CR file is used for 2 things (a) for an example CR that users can refer to, each value in the example CR file is the default, so users can use this as a reference for all default values, and
(b) this file is used to test the schema validation - our CI action runs a validation test on the example CR which confirms the schema and example files are both valid.

So in this file: https://github.com/kiali/kiali-operator/blob/v2.1.0/crd-docs/cr/kiali.io_v1alpha1_kiali.yaml#L437

you need to add this:

        settings:
          animation: "point"
          font_label: 13
          min_font_badge: 7
          min_font_label: 10

@jshaughn
Copy link
Contributor Author

jshaughn commented Nov 25, 2024

You forgot to add the default to the example CR, and I just noticed we are missing all of the settings here

I will add the new one, but the others are actually purposefully unadvertised, and also will go away with the cytoscape
deprecation.

Having said that, I see that we are missing:

traffic:
  ambient: "total"

I'll add that, too.

- add missing settings
@jmazzitelli
Copy link
Contributor

Now that CR validation failed.
https://github.com/kiali/kiali-operator/actions/runs/12011956788/job/33482097244?pr=851

Error from server (BadRequest): error when creating "STDIN": TestKiali in version "v1alpha1" cannot be handled as a TestKiali: strict decoding error: unknown field "spec.kiali_feature_flags.ui_defaults.graph.traffic.ambient"

@jshaughn
Copy link
Contributor Author

Flibbedy floo, looks like the ambient setting was missing all over the place. I'll add it.

- now need to add other missing ambient traffic entries
Copy link
Contributor

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

validation passes. lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test: n/a PR does not need test additions or updates
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants