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

support mutli images #230

Merged
merged 2 commits into from
Sep 16, 2020
Merged

support mutli images #230

merged 2 commits into from
Sep 16, 2020

Conversation

qixiaogang
Copy link
Contributor

@qixiaogang qixiaogang commented Aug 17, 2020

Change log description

Add image and volume array in the crd to support customize images deploy with zookeeper node.

Purpose of the change

Support multi images, issue: #228

What does the code do

This code allows users to add customized images and volumes in the pod spec, which means users can deploy other containers with the zookeeper.

To support multi containers, add containers and volumes to the manifest
spec: containers: # array of Container in k8s.io/api/core/v1 volumes: # array of Volume in k8s.io/api/core/v1

How to verify it

Create a ZookeeperCluster CR and add another image to the containers and define the volume in the volumes, then apply the ZookeeperCluster CR, you will find the zookeeper instance with start with the images define in the containers.

@qixiaogang qixiaogang mentioned this pull request Aug 17, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2020

Codecov Report

Merging #230 into master will decrease coverage by 0.65%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
- Coverage   81.81%   81.16%   -0.66%     
==========================================
  Files          11       11              
  Lines        1248     1258      +10     
==========================================
  Hits         1021     1021              
- Misses        162      170       +8     
- Partials       65       67       +2     
Impacted Files Coverage Δ
...g/apis/zookeeper/v1beta1/zookeepercluster_types.go 98.30% <ø> (ø)
...kg/apis/zookeeper/v1beta1/zz_generated.deepcopy.go 82.03% <0.00%> (-4.19%) ⬇️
pkg/zk/generators.go 98.31% <100.00%> (ø)

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 b5502fb...555d54f. Read the comment docs.

@Prabhaker24
Copy link
Contributor

@qixiaogang can you please add the steps for verification in the pr?

@qixiaogang
Copy link
Contributor Author

@Prabhaker24 I doesn't get you about "the steps for verification in the PR", can you share more details?

and sorry for reply so late.

@qixiaogang
Copy link
Contributor Author

@Prabhaker24 Can you help with above comments

@Prabhaker24
Copy link
Contributor

@qixiaogang sorry I got busy with some other stuff. What I mean is you have removed the sections What the code does and How to verify from your pr description can you please add them back and explain what the code is doing there you can refer the following pr's description #215, Thanks.

@qixiaogang
Copy link
Contributor Author

Thanks, get it.

@qixiaogang
Copy link
Contributor Author

@Prabhaker24 Add the What the code does and How to verify back, please check

Copy link
Contributor

@Prabhaker24 Prabhaker24 left a comment

Choose a reason for hiding this comment

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

LGTM

@qixiaogang
Copy link
Contributor Author

@anishakj Can you help to review the PR, thanks

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.

@qixiaogang Could you please add the new fields in https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper/README.md, and https://github.com/pravega/zookeeper-operator/blob/master/charts/zookeeper/values.yaml

Also, since new parameters are added in spec ,CRD have to be changed to include the changes

@qixiaogang
Copy link
Contributor Author

@anishakj Add new scripts in the two files and update the crd

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

@anishakj anishakj merged commit 80e41b1 into pravega:master Sep 16, 2020
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.

4 participants