-
Notifications
You must be signed in to change notification settings - Fork 85
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(helm): Add p2p support to helm chart #2187
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2187 +/- ##
==========================================
+ Coverage 65.79% 65.85% +0.05%
==========================================
Files 139 139
Lines 18286 18287 +1
Branches 18286 18287 +1
==========================================
+ Hits 12032 12042 +10
+ Misses 4964 4955 -9
Partials 1290 1290 ☔ View full report in Codecov by Sentry. |
@@ -0,0 +1,21 @@ | |||
{{- if and (.Values.backup.enabled) .Values.p2p.enabled }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why should backup
be enabled for p2p mode?
apiVersion: v1 | ||
kind: Service | ||
metadata: | ||
name: {{ template "papyrus.name" . }}-p2p-svc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to add svc
suffix (no need to mention the resource type in the resource name)
deployments/helm/values.yaml
Outdated
network_bootstrap_peer_multiaddr_is_none: | ||
# Mandatory - The bootstrap server to connect to, ip adress | ||
network_bootstrap_peer_multiaddr_ip: | ||
# Mandatory - The bootstrap server to connect to, port | ||
network_bootstrap_peer_multiaddr_port: | ||
# Mandatory - The bootstrap server to connect to, uid | ||
network_bootstrap_peer_multiaddr_uid: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since all these params have similar purpose and are all related, I suggest to nest them together:
bootstrap_server:
ip:
port:
uid:
also, we need to be consistent in the syntax formatting. either sneak_case
or camelCase
but not both
deployments/helm/values.yaml
Outdated
network_bootstrap_peer_multiaddr_uid: | ||
service: | ||
# Specify service type, options are ClusterIP | NodePort | LoadBalancer | ||
type: LoadBalancer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we currently support service types other then NodePort?
deployments/helm/values.yaml
Outdated
@@ -34,21 +70,30 @@ deployment: | |||
requests: | |||
cpu: 500m | |||
memory: 1Gi | |||
## Optionally specify extra environment variables to add to papyrus container | |||
extraEnv: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the word extra
in extraEnv
implies that we have some default/pre-existing env vars we support. since we don't have such we don't need the extra
part.
Previously, eranreshef-starkware (Eran Reshef) wrote…
Yes, you can use ClusterIP, LoadBalancer and NodePort. |
Previously, idan-starkware (Idan Shamam) wrote…
so if I currently set this field to any value other then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r3, 4 of 5 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @idan-starkware and @RoeiE-starkware)
8f576a6
to
009ca93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @idan-starkware and @RoeiE-starkware)
deployments/helm/values.yaml
line 35 at r8 (raw file):
# Mandatory - The network.#is_none flag on the bootsrap server multiaddrIsNone: # Mandatory - The bootstrap server to connect to, ip adress
address
Code quote:
adress
This change is