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

Issue 233: Fixed chart bug and made ready and unready fields as nullable and added checks for various crd fields #234

Merged
merged 7 commits into from Sep 3, 2020

Conversation

Prabhaker24
Copy link
Contributor

@Prabhaker24 Prabhaker24 commented Aug 27, 2020

Signed-off-by: prabhaker24 prabhaker.saxena@dell.com

Change log description

Create zookeeper cluster is failing with chart due to domain name not specified correctly in charts Ater the OpenApi SchemaValidation check Added and made the ready and unready member list as nullable in crd.

Purpose of the change

Fixes #233 #208

What the code does

Because of the OpenApiSchemaValidation being present we can't have the domainname with null value which was the case till now so I have changed it so that if no value is provided domainname field get's skipped along with that to keep backwards compatibility that is for the latest operator code to be able to deploy zookeeper cluster 0.2.8 or before we need to make ready and unready member list as nullable in the crd as they were not omitempty before.

Additionally, I have removed controller-gen command from make file and have used operator-sdk command to create the crd below is the command:-

operator-sdk generate crds

And have added validation checks for various fields in the crd.

How to verify it

Create zookeeper cluster using charts after deploying the zookeeper-operator, Cluster should get deployed.

Signed-off-by: prabhaker24 <prabhaker.saxena@dell.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2020

Codecov Report

Merging #234 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #234   +/-   ##
=======================================
  Coverage   81.81%   81.81%           
=======================================
  Files          11       11           
  Lines        1248     1248           
=======================================
  Hits         1021     1021           
  Misses        162      162           
  Partials       65       65           
Impacted Files Coverage Δ
pkg/apis/zookeeper/v1beta1/status.go 100.00% <ø> (ø)
...g/apis/zookeeper/v1beta1/zookeepercluster_types.go 98.30% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bd5bbf...326e5ce. Read the comment docs.

prabhaker24 added 3 commits August 28, 2020 09:19
Signed-off-by: prabhaker24 <prabhaker.saxena@dell.com>
Signed-off-by: prabhaker24 <prabhaker.saxena@dell.com>
Signed-off-by: prabhaker24 <prabhaker.saxena@dell.com>
Copy link
Contributor

@anishakj anishakj left a comment

Choose a reason for hiding this comment

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

Please add validation spec fields like pullpolicy, container port etc. And test the negative scenarios and ensure proper error is given

prabhaker24 added 2 commits September 2, 2020 20:35
Signed-off-by: prabhaker24 <prabhaker.saxena@dell.com>
Signed-off-by: prabhaker24 <prabhaker.saxena@dell.com>
@anishakj
Copy link
Contributor

anishakj commented Sep 3, 2020

Could you please check can we add validations for storagetype and access modes

Signed-off-by: prabhaker24 <prabhaker.saxena@dell.com>
@Prabhaker24 Prabhaker24 changed the title Issue 233: Fixed chart bug and removed ready and unready fields from crd Issue 233: Fixed chart bug and made ready and unready fields as nullable and added checks for various crd fields Sep 3, 2020
Copy link
Contributor

@anishakj anishakj left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Create zookeeper cluster is failing with chart due to domain name not specified correctly in charts
3 participants