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

KIALI-2488 Composition mechanism for dashboards #959

Merged
merged 2 commits into from Apr 2, 2019

Conversation

@jotak
Copy link
Contributor

commented Mar 29, 2019

  • Add new "Include" field in custom resource
  • Split some dashboards (JVM based, etc.), using composition
  • Add tests; prevent circular dependency
  • Loosing compatibility with previous model (breaking changes), but should not have much impact (see PR discussion)

JIRA: https://issues.jboss.org/browse/KIALI-2488

@hhovsepy

This comment has been minimized.

Copy link

commented Mar 29, 2019

cc @skondkar as a QE for all dashboard verifications

@jotak jotak force-pushed the jotak:kiali-2488 branch from 43a8f2d to 74b34cf Mar 29, 2019
@jotak jotak requested review from lucasponce and pilhuhn Mar 29, 2019
@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@pilhuhn @lucasponce for easier reviewing, I've split the PR in two commits :
9c62f13 is about code changes and 74b34cf is dashboards yaml changes

spans: 3
metricName: "base:gc_complete_scavenger_count"
dataType: "raw"
- include: "microprofile-x.y"

This comment has been minimized.

Copy link
@jotak

jotak Mar 29, 2019

Author Contributor

@pilhuhn can you give me the version of microprofile that you used to create this dashboard, so that I can update its name?

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

Jenkins CI: kiali-core-pr-e2e-test #762

  • ✔️ run-kiali-e2e-tests #[1558]
DisplayName string `json:"displayName"`
}

// TODO: auto-generate the following deepcopy methods!

This comment has been minimized.

Copy link
@jotak

jotak Mar 29, 2019

Author Contributor

@lucasponce I don't know what you think about that, maybe at some point we should add some code-generation tool to our build workflow to generate such code (that would also allow us to use kind-of generics next time we need it!)

This comment has been minimized.

Copy link
@lucasponce

lucasponce Apr 2, 2019

Contributor

+1 yeah, it's what I see most projects do.
Also, I saw that can be added as an "annotation" driven by the go generate phase.
(It's what I saw also in Istio while tackling with the adapters code).

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

Jenkins CI: kiali-core-pr-e2e-test #763

  • ✔️ run-kiali-e2e-tests #[1559]
result := v1alpha2.MonitoringDashboard{}
err := in.clientV1Alpha2.Get().Namespace(namespace).Resource("monitoringdashboards").SubResource(name).Do().Into(&result)
if errors.IsNotFound(err) {
// Try with older version

This comment has been minimized.

Copy link
@jotak

jotak Mar 29, 2019

Author Contributor

@lucasponce if you have any suggestion here to do better, I'm all ears. Basically, to allow backward compatibility, I'm trying to get dashboards first with client v1alpha2, if not found I retry with client for v1alpha1.
But maybe we can avoid having this extra call to a client.
AFAIK, when the "monitoringdashboards" CDR is installed on a cluster, I think it's only for 1 version (so we cannot have at the same time different versions of resources). So maybe there's a way to check what CRD is installed on the cluster to use directly the right client? Any idea?

@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

cc @skondkar as a QE for all dashboard verifications

FYI there is no visual difference expected with the current dashboards (or very minor, like maybe I changed the order of some charts in thorntail dashboard). So I guess this is mainly for non-regression tests

@lucasponce

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

do we need an v1alpha2 ? I mean, as it's pretty v1alpha1 and this feature is still being in active development I wouldn't seen bad to maintain the v1alpha1 and apply changes there, to not deal with backward compatibility yet.

I remember that Istio 0.x maintained the v1alpha1 and it changed the CRD within same v1alpha1.

I only comment this, if that would help to simplify scenarios to maintain v1alpha1 and v1alpha2 at same time.

@skondkar

This comment has been minimized.

Copy link

commented Mar 29, 2019

Had some manual tests in general on dashboards. looks fine.
kiali-ui 0.17.0 (fa3cc76a6429b4bb292f1a2290f77e4697c4bbe0)
kiali v0.17.0-SNAPSHOT (cdd719c)

@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@lucasponce I don't remember how istio dealed with it, but here there is some breaking change in the model that's why I am increasing the version number. So that a dashboard that has been previously used is still going to work with the new code (even if maybe no-one is using it, dunno ... but that just seems more correct to me). Maintaining backward compatibility is not that difficult here, as v1alpha1 can be converted to v1alpha2. There's just that question about detecting the version is use based on the installed CRD, but even if we don't have to solve that question now, we will have anyway to solve it sooner or later... so why not now?

@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

(I can also see this as some "training" in dealing with versionned crd)

@lucasponce

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Istio 0.x performed all changes in the CRDs under same v1alpha1, until reached the 1.0.
It provided a cli / script mechanism to upgrade versions between 0.x.

Adding a new version sounds correct, but in the other side, an "alpha" is a state where user may expect changes.

Also, a new version is not cheap, it needs an additional client (not a big deal) but I wonder if it's better to maintain same v1alpha1 until we get more traction on it and then we can consolidate and bump version.

IMO I'd use a new version when the feature is more consolidated.

@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

ok, fair enough :) I'll cancel the version bump.
@pilhuhn sounds good to you too?

jotak added 2 commits Mar 29, 2019
- Add new "Include" field in custom resource
- Add tests; prevent circular dependency
- Loosing compatibility with previous model (breaking changes), but should not have much impact (see PR discussion)
Split some dashboards (JVM based, etc.), using composition
@jotak jotak force-pushed the jotak:kiali-2488 branch from 74b34cf to bb7558c Apr 1, 2019
@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

So, I removed the version bump and push again

TBH I'm fine with not handling versions not because Istio did it in the past (they might had their own reasons to do so), but more because we estimate the dashboards are not much being used yet so there's good chances it passes unnoticed. Just in case, I'm keeping locally a branch of the code with version bump.

@lucasponce

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

TBH I'm fine with not handling versions not because Istio did it in the past (they might had their own reasons to do so), but more because we estimate the dashboards are not much being used yet so there's good chances it passes unnoticed. Just in case, I'm keeping locally a branch of the code with version bump.

+1

I'm starting to review your last version :-). Thanks.

Copy link
Contributor

left a comment

LGTM
Tested locally and code looks correct.

@lucasponce lucasponce merged commit e47974d into kiali:master Apr 2, 2019
3 checks passed
3 checks passed
Jenkins-CI Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - tests/e2e/requirements.txt (theute) No manifest changes detected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.