Skip to content

Add elasticsearch and kibana CRD generation for via operator-sdk #442

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 4 commits into from
Aug 7, 2020
Merged

Add elasticsearch and kibana CRD generation for via operator-sdk #442

merged 4 commits into from
Aug 7, 2020

Conversation

@periklis
Copy link
Contributor

@periklis periklis commented Aug 3, 2020 •
edited
Loading

Description

This PR addresses the required annotations in elasticsearch_types.go and kibana_types.go to enable almost identical but backward-compatible generated CRDs by operator-sdk generate crds. In addition it applies oc kustomize patches to suppress validation that did not exist in the former CRD (e.g. status field) and in turn maintain backward compatibility. Remain differences can be inspected here:

In detail, the changes included:

  • The CRD generation ensures apiVersion v1beta1 for Elasticsearch and Kibana.
  • Transfers all descriptions from yaml file into {elasticsearch,kibana}_types.go
  • Maintains all nullable and optional field validations.
  • Maintains all enum field validations where provided in yaml file, i.e. only for ManagementState.
  • Maintains generic status validation as type: object by applying kustomize patch

Additional changes in separate commits:

  • Fix opm tool pining to generate operator bundle.
  • Re-generate operator bundle to include generated CRDs and drop old handwritten ones.

About remaining non-breaking CRD validation differences

  1. Field spec includes a new section versions which represents the defaults for this field and thus not breaking the API. ✔️
  2. Field openAPIV3Schema includes the defaults for apiVersion, kind and description. ✔️
  3. Field kibana.properties.proxySpec is replaced with the correct kibana.properties.proxy, was broken before. ✔️
  4. Fields resources.properties.{limits,requests} include additional validations for the string schema (e.g. 1Gi, 100m). This change is an addition and is not breaking existing API objects, because resource.MustParse(...) ensured this already. Affects kibana, kibana.proxy, elastiscearch, elasticsearch.proxy. ✔️
  5. Field nodeSelector includes additional property checks for string schema. This change is an addition and is not breaking existing API objects. Affects kibana, elastiscearch. ✔️
  6. Field tolerations includes validations for the golang struct v1.Toleration. This change is an enhancement and not breaking the API object, because type serialization was in place. Affects kibana, elasticsearch. ✔️

Further enhancements

This PR adds informative printer columns for elasticsearch and kibana CRs. In addition both CRs are grouped under the category logging and elasticsearch also under tracing. Example:

image

Further notes
The CRD comparison is based on the former and the generated yaml files using dyff v1.0.2

/cc @ewolinetz @blockloop

ewolinetz
ewolinetz reviewed Aug 3, 2020
View reviewed changes
ewolinetz
ewolinetz reviewed Aug 3, 2020
View reviewed changes
ewolinetz
ewolinetz reviewed Aug 3, 2020
View reviewed changes
ewolinetz
ewolinetz reviewed Aug 3, 2020
View reviewed changes
ewolinetz
ewolinetz reviewed Aug 3, 2020
View reviewed changes
pkg/apis/logging/v1/elasticsearch_types.go
// Management spec for indicies
//
// +nullable
// +optional
Copy link
Contributor

@ewolinetz ewolinetz Aug 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

so, technically we won't create our indices without this.. so i'm not sure it should be optional (though i think it is optional for CLO because we create defaults there otherwise)
@jcantrill thoughts?

ewolinetz
ewolinetz reviewed Aug 3, 2020
View reviewed changes
ewolinetz
ewolinetz reviewed Aug 3, 2020
View reviewed changes
@periklis periklis force-pushed the crd-generation branch 3 times, most recently from 9690501 to f78c8e8 Compare August 4, 2020 09:24
@periklis
Copy link
Contributor Author

periklis commented Aug 4, 2020

/retest

@periklis
Copy link
Contributor Author

periklis commented Aug 4, 2020

/retest

2 similar comments
@periklis
Copy link
Contributor Author

periklis commented Aug 5, 2020

/retest

@periklis
Copy link
Contributor Author

periklis commented Aug 5, 2020

/retest

@periklis periklis mentioned this pull request Aug 5, 2020
Merged
@periklis
Copy link
Contributor Author

periklis commented Aug 5, 2020

/retest

1 similar comment
@periklis
Copy link
Contributor Author

periklis commented Aug 5, 2020

/retest

@ewolinetz
Copy link
Contributor

ewolinetz commented Aug 5, 2020

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2020
@periklis
Copy link
Contributor Author

periklis commented Aug 6, 2020

/retest

@periklis
Copy link
Contributor Author

periklis commented Aug 6, 2020

/retest

3 similar comments
@periklis
Copy link
Contributor Author

periklis commented Aug 6, 2020

/retest

@periklis
Copy link
Contributor Author

periklis commented Aug 6, 2020

/retest

@periklis
Copy link
Contributor Author

periklis commented Aug 6, 2020

/retest

ewolinetz
ewolinetz reviewed Aug 6, 2020
View reviewed changes
bundle/manifests/crd-drop-status-validation-patch.yaml
@@ -0,0 +1,4 @@
- op: replace
Copy link
Contributor

@ewolinetz ewolinetz Aug 6, 2020

Choose a reason for hiding this comment

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

we should exclude this from the bundle manifests file

Copy link
Contributor

@ewolinetz ewolinetz Aug 6, 2020

Choose a reason for hiding this comment

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

ignore this -- will fix as part of #446

ewolinetz
ewolinetz reviewed Aug 6, 2020
View reviewed changes
bundle/manifests/kustomization.yaml
@@ -0,0 +1,16 @@
resources:
Copy link
Contributor

@ewolinetz ewolinetz Aug 6, 2020

Choose a reason for hiding this comment

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

we should exclude this from the bundle manifests file

@ewolinetz
Copy link
Contributor

ewolinetz commented Aug 6, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2020
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 6, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ewolinetz, periklis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

9 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 6, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 7, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 7, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 7, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 7, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 7, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 7, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit d19ea87 into openshift:master Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blockloop blockloop Awaiting requested review from blockloop

1 more reviewer

@ewolinetz ewolinetz ewolinetz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ewolinetz ewolinetz

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@periklis @ewolinetz @openshift-ci-robot @openshift-bot @openshift-merge-robot