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

[5.10.x] Helm deployment pattern 2 #268

Merged
merged 16 commits into from
Nov 21, 2020
Merged

Conversation

darsham3
Copy link

@darsham3 darsham3 commented Nov 10, 2020

Changes

This PR adds Helm chart deployment pattern 2 for IS 5.10 with Identity Analytics 5.8.

Related Issue - #270

@CLAassistant
Copy link

CLAassistant commented Nov 10, 2020

CLA assistant check
All committers have signed the CLA.

@@ -0,0 +1,6 @@
dependencies:
Copy link
Contributor

Choose a reason for hiding this comment

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

@darsham3 is this required file ?

@@ -0,0 +1,13 @@
<?xml version='1.0' encoding='UTF-8' standalone='no'?>
Copy link
Contributor

Choose a reason for hiding this comment

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

@darsham3 is this required ? seems file a duplicate file to sessiondata publisher.

@@ -0,0 +1,20 @@
# Copyright (c) 2018, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 shall we please update the license header year to latest? Please update the other files if applicable.

Copy link
Author

Choose a reason for hiding this comment

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

updated as 2020 in all relevant files

- Deploy the Kubernetes resources using the Helm Chart

```
helm install <RELEASE_NAME> <HELM_HOME>/is-pattern-1 --version 5.10.0-2 --namespace <NAMESPACE> --dependency-update
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 please update the pattern name in the instruction. Please fix similar issues if there are any other.

- Deploy the Kubernetes resources using the Helm Chart

```
helm install <RELEASE_NAME> <HELM_HOME>/is-pattern-1 --version 5.10.0-2 --namespace <NAMESPACE> --dependency-update
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 please update the release version to 5.10.0-3 as this will be the next release number. Please fix applicable, similar scenarios.


The output under the relevant column stands for the following.

- NAME: Metadata name of the Kubernetes Ingress resource (defaults to `wso2is-pattern-1-identity-server-ingress`)
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 it would be better to add the Dashboard hostname details and instructions associated with it as well. Please sync this with the NOTES.txt output.

dependencies:
- name: mysql-is
version: "5.10.0-2"
repository: file://../databases/mysql-is/
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 please refer to the official repository in the commit instead of the local charts directory file.


{{- define "fullname" -}}
{{- "wso2is-pattern-2" }}
{{- end -}}
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 please add a new EOF line. Please check other files as well. If any, please fix them.

configMap:
name: is-analytics-worker-carbon-conf
{{ end }}

Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 please remove this unnecessary EOF line.

namespace : {{ .Release.Namespace }}
spec:
replicas: {{ default 1 .Values.wso2.deployment.wso2isAnalyticsWorker2.replicas }}
# strategy:
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 please remove this since it is not needed.

{{ . }}
{{- end }}
{{- end }}

Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 please remove this unnecessary EOF line.

@@ -0,0 +1,97 @@
{{/*
Copyright (c) 2019, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

License header year should be 2020.

{{/*
Expand the name of the chart.
*/}}
{{- define "is-pattern-1.name" -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have pattern-1 related values here?

Copy link
Author

Choose a reason for hiding this comment

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

removed unnecessary values.

namespace : {{ .Release.Namespace }}
spec:
replicas: {{ default 1 .Values.wso2.deployment.wso2isAnalyticsWorker1.replicas }}
# strategy:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we please remove commented lines if not required.

description: A Helm chart for the deployment of WSO2 Identity And Access Management pattern 2
name: is-pattern-2
version: 5.10.0-3
icon: https://wso2.cachefly.net/wso2/sites/all/images/wso2logo.svg
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 please add a new EOF line.

@@ -0,0 +1,285 @@
# Helm Chart for a clustered deployment of WSO2 Identity Server
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 I suppose the pattern name has to be ...Identity Server with Analytics Support.

<property name="receiverURL">tcp://wso2is-pattern-2-analytics-worker-1-service:7612|tcp://wso2is-pattern-2-analytics-worker-2-service:7612</property>
<property encrypted="false" name="password">admin</property>
</to>
</eventPublisher>
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 please add a new line at EOF.

advanced/is-pattern-2/requirements.yaml Show resolved Hide resolved
namespace : {{ .Release.Namespace }}
spec:
replicas: {{ default 2 .Values.wso2.deployment.wso2isAnalyticsDashboard.replicas }}
# strategy:
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 let's leave the strategy section without keeping it commented out. No need to maintain commented out configurations.

Plus, we can assume that we are using the default StatefulSet update strategy, Kubernetes RollingUpdates.

@@ -0,0 +1,31 @@

Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 please remove this extra line.

imagePullPolicy: Always

# Number of deployment replicas
replicas: 1
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 why are we having a single replica by default? We can go ahead with 2 replicas by default.

name: {{ template "fullname" . }}-analytics-worker-statefulset
namespace : {{ .Release.Namespace }}
spec:
replicas: {{ default 1 .Values.wso2.deployment.wso2isAnalyticsWorker.replicas }}
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 no need to define default values at the source level. We can depend on the values provided at the values.yaml file, as the default values.

Please apply the same to other values, where necessary.

@@ -0,0 +1,193 @@
# Copyright (c) 2020, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 for the Dashboard the ideal option is to use a Kubernetes Deployment definition. Can we use a Deployment definition in this case?

Copy link
Author

Choose a reason for hiding this comment

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

updated

clusterIP: None
selector:
deployment: {{ template "fullname" . }}-analytics-worker
node: node-1
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 we don't need this label. Only deployment: {{ template "fullname" . }}-analytics-worker is enough when mapping to the service.

deployment: {{ template "fullname" . }}-analytics-worker
app: is-pattern-2
monitoring: {{ .Values.wso2.monitoring.prometheus.jmxJobName }}
node: node-1
Copy link
Member

Choose a reason for hiding this comment

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

@darsham3 we don't need this label.

@chirangaalwis chirangaalwis merged commit a6e907e into wso2:5.10.x Nov 21, 2020
@chirangaalwis
Copy link
Member

@darsham3 appreciate if you can create an issue for this and link in this PR.

Also, in order to get this released please update the README documentation and change log documentation.

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