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(helm): Add p2p support to helm chart #2187

Closed
wants to merge 20 commits into from

Conversation

idan-starkware
Copy link
Contributor

@idan-starkware idan-starkware commented Jul 2, 2024

  • Modified the helm templates to support extra environment variables.
  • Unified multiple svc creation into single service with multiple ports.

This change is Reviewable

@idan-starkware idan-starkware self-assigned this Jul 2, 2024
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.85%. Comparing base (c11022c) to head (7592d40).
Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@idan-starkware idan-starkware changed the title Added support to extraEnv in templates. modified svc template Added support to extraEnv in templates Jul 2, 2024
@idan-starkware idan-starkware changed the title Added support to extraEnv in templates feat(deployment): Added support to extraEnv in templates Jul 3, 2024
@idan-starkware idan-starkware changed the title feat(deployment): Added support to extraEnv in templates feat(deployment): Add p2p support to helm chart Jul 7, 2024
@idan-starkware idan-starkware changed the title feat(deployment): Add p2p support to helm chart chore(deployment): Add p2p support to helm chart Jul 7, 2024
@@ -0,0 +1,21 @@
{{- if and (.Values.backup.enabled) .Values.p2p.enabled }}
Copy link
Collaborator

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
Copy link
Collaborator

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/templates/service.yaml Outdated Show resolved Hide resolved
Comment on lines 33 to 39
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:
Copy link
Collaborator

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

network_bootstrap_peer_multiaddr_uid:
service:
# Specify service type, options are ClusterIP | NodePort | LoadBalancer
type: LoadBalancer
Copy link
Collaborator

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?

@@ -34,21 +70,30 @@ deployment:
requests:
cpu: 500m
memory: 1Gi
## Optionally specify extra environment variables to add to papyrus container
extraEnv: []
Copy link
Collaborator

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.

@idan-starkware
Copy link
Contributor Author

deployments/helm/values.yaml line 42 at r7 (raw file):

Previously, eranreshef-starkware (Eran Reshef) wrote…

do we currently support service types other then NodePort?

Yes, you can use ClusterIP, LoadBalancer and NodePort.
We can later add additional support to specific loadbalancer configurations and service annotations but its not required for now.

@eranreshef-starkware
Copy link
Collaborator

deployments/helm/values.yaml line 42 at r7 (raw file):

Previously, idan-starkware (Idan Shamam) wrote…

Yes, you can use ClusterIP, LoadBalancer and NodePort.
We can later add additional support to specific loadbalancer configurations and service annotations but its not required for now.

so if I currently set this field to any value other then nodePort, will it work?

Copy link
Collaborator

@eranreshef-starkware eranreshef-starkware left a 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)

Copy link
Collaborator

@eranreshef-starkware eranreshef-starkware left a 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

@idan-starkware idan-starkware requested review from eranreshef-starkware and removed request for RoeiE-starkware July 9, 2024 13:12
@idan-starkware idan-starkware changed the title chore(deployment): Add p2p support to helm chart chore(helm): Add p2p support to helm chart Jul 9, 2024
@idan-starkware idan-starkware deleted the idan/papyrus-p2p-deployment branch July 9, 2024 15:06
@github-actions github-actions bot locked and limited conversation to collaborators Jul 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants