-
Notifications
You must be signed in to change notification settings - Fork 951
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
Add helm-charts for explorer-db & explorer #353
base: main
Are you sure you want to change the base?
Conversation
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.
Please have the default values for the parameters.
875038a
to
f454267
Compare
Signed-off-by: vineeth2328 <[email protected]>
f454267
to
4998ea4
Compare
{{- toYaml .Values.annotations | nindent 6 }} | ||
data: | ||
|
||
test-network.json: |- |
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.
can we rename it something more meaningful?
app: {{ .Release.Name }}-app | ||
ports: | ||
- port: {{ .Values.explorer.service.port }} | ||
targetPort: {{ .Values.explorer.service.port }} |
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.
targetPort is the port on which explorer will be running internally, and to use the custom value for it, we also need the explorer to run on the same port. Can you check once if we can run explorer on the custom port.
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.
By default explorer port running on 8080. if we want to use custom value for targetPort like 9090. we need to add
PORT: 9090 in env of explorer.
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.
can you check if this property is available in the configmap
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.
Not their In current configmap. if want I can update
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.
if we are updating the targetPort in the service then we need to add that, or the other way is to keep the value of target port to 8080
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.
Okay I'll update configmap & default value as 8080. is that fine?
Signed-off-by: vineeth2328 <[email protected]>
Signed-off-by: vineeth2328 <[email protected]>
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.
Add suport for PVC
Signed-off-by: Aditya Joshi <[email protected]>
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 helm chart looks good, except for leaking secret information when used. We could proceed with two options, one is to accept this change as is and tag it for non-production deployments. The other option is to improve it further and then release it.
} | ||
} | ||
}, | ||
"organizations": { |
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.
Something to improve upon later: One instance of Explorer can be used for multiple organizations. There can be a different user login session seeing altogether a different network configuration. This section shall be more flexible to handle that.
"{{ .Values.network.mspid}}": { | ||
"mspid": "{{ .Values.network.mspid}}", | ||
"adminPrivateKey": { | ||
"pem": "{{ .Values.network.adminPrivateKey}}" |
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.
Please load private key from either a secret store or create in-memory volume to load it from an external secret storage.
DATABASE_HOST: {{ .Release.Name }}-db | ||
DATABASE_DATABASE: {{ $.Values.db.database | default "fabricexplorer" }} | ||
DATABASE_USERNAME: {{ $.Values.db.database_username | default "hppoc" }} | ||
DATABASE_PASSWD: {{ $.Values.db.database_password | default "password" }} |
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.
Similar to the private key, the password parameter is sensitive, it shall be loaded in from a secret store.
data: | ||
DATABASE_DATABASE: {{ $.Values.db.name | default "fabricexplorer" }} | ||
DATABASE_USERNAME: {{ $.Values.db.username | default "hppoc" }} | ||
DATABASE_PASSWORD: {{ $.Values.db.password | default "password" }} |
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.
Kubernetes secrets is a better place to load the secret values such as password.
helm-charts/explorer/values.yaml
Outdated
#Eg. peerurl: grpcs://peer0-org1:7051 | ||
peerurl: | ||
#Provide the adminPrivateKey | ||
#Eg. adminPrivateKey: -----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgUH2s+pegkyjaef+X\nyYBBP52PANixdkt7+xRgTkVPQPahRANCAATChVh6QCmBtI4/6S93XfEj7EDlz0Tf\nISNDAB3rsyjj3T60QhRGczSQmFyEkx6nrrWNyXLsTe/gr4Cc0l//QKQp\n-----END PRIVATE KEY-----\n |
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.
Secret information, let us evaluate if an alternate option can be used.
Signed-off-by: Vineeth Boppudi <[email protected]>
PR is of interest for bevel, we were exploring opt. to add helm charts. Is this being currently worked on ? |
What this PR does / why we need it:
This feature implements helm-charts for explorer-db & explorer
Which issue(s) this PR fixes:
Fixes #333
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation, usage docs, etc.: