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

chore: upgrade zk to v1.0 api, refactor script and add ut #1097

Merged
merged 9 commits into from
Oct 21, 2024

Conversation

Y-Rookie
Copy link
Contributor

No description provided.

@Y-Rookie Y-Rookie force-pushed the support/zookeeper-v1.0-upgrade branch from 5f0bd28 to 9471421 Compare October 17, 2024 03:31
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 161 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (7ba24ff) to head (789111c).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
addons/zookeeper/scripts-ut-spec/startup_spec.sh 0.00% 99 Missing ⚠️
addons/zookeeper/scripts-ut-spec/roleprobe_spec.sh 0.00% 62 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #1097    +/-   ##
======================================
  Coverage   0.00%   0.00%            
======================================
  Files         22      24     +2     
  Lines       3576    3737   +161     
======================================
- Misses      3576    3737   +161     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kubeJocker
kubeJocker previously approved these changes Oct 17, 2024
Copy link
Collaborator

@kubeJocker kubeJocker left a comment

Choose a reason for hiding this comment

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

LGTM

value: {{ .Values.zookeeper.dataLogDir }}
- name: ZOOBINDIR
value: "/opt/bitnami/zookeeper/bin"
- name: ZOOBIN
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this var useful? Its value is the same as the upper one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this var useful? Its value is the same as the upper one.

It should be necessary. This addon uses the Bitnami image, and the Bitnami image internally has some dependent environment variables (env).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this var useful? Its value is the same as the upper one.

It should be necessary. This addon uses the Bitnami image, and the Bitnami image internally has some dependent environment variables (env).

@kubeJocker please take a look

Copy link
Collaborator

Choose a reason for hiding this comment

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

This var is used in the ZkServer.sh of version 3.4, so it is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, please add a comment as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, please add a comment as well.

done

value: "/opt/bitnami/zookeeper/bin"
- name: ZOO_ENABLE_AUTH
value: "yes"
- name: SERVICE_PORT
Copy link
Contributor

Choose a reason for hiding this comment

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

This var seems useless.

Copy link
Collaborator

Choose a reason for hiding this comment

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

service_port needs to be preserved as Gemini will utilize this variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment to the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please add a comment to the code.

done

@Y-Rookie Y-Rookie merged commit cd48e0e into main Oct 21, 2024
27 checks passed
@Y-Rookie Y-Rookie deleted the support/zookeeper-v1.0-upgrade branch October 21, 2024 04:03
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.

5 participants