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

Release helm chart #3312

Merged
merged 8 commits into from Dec 7, 2020
Merged

Release helm chart #3312

merged 8 commits into from Dec 7, 2020

Conversation

mforutan
Copy link
Contributor

@mforutan mforutan commented Dec 6, 2020

Using GitHub Actions to release the official helm chart

This PR is a starting point to introduce an official helm chart for locakstack,

I copied the helm chart from sidhuko/charts#1 without any modification (hope it is fine with @sidhuko)

There are couple of tasks that need to be done before this can be merged:

  • A gh-pages branch need to be created
  • GitHub Pages need to be enabled in the setting

TODO (can be done in this PR or next PRs):

  • A GitHub workflow to test the helm chart for PRs

Fixes #882

@mforutan mforutan mentioned this pull request Dec 6, 2020
@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.111% when pulling da208f9 on mforutan:add-helm-chart into 9a4f516 on localstack:master.

{{- end }}
livenessProbe:
httpGet:
path: /
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this can we use something like /health for health check path?

port: http
readinessProbe:
httpGet:
path: /
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this can we use something like /health for health check path?

Comment on lines +14 to +111
- name: apigateway
enabled: false
endpoint: ""
internalPort: 4567
externalPort: 4567
nodePort: 31567
- name: cloudformation
enabled: false
endpoint: ""
internalPort: 4581
externalPort: 4581
nodePort: 31581
- name: dynamodb
enabled: false
endpoint: ""
internalPort: 4569
externalPort: 4569
nodePort: 31569
- name: elasticsearch
enabled: false
endpoint: ""
internalPort: 4571
externalPort: 4571
nodePort: 31571
- name: kinesis
enabled: false
endpoint: ""
internalPort: 4568
externalPort: 4568
nodePort: 31568
- name: s3
enabled: false
endpoint: ""
internalPort: 4572
externalPort: 4572
nodePort: 31572
- name: sns
enabled: false
endpoint: ""
internalPort: 4575
externalPort: 4575
nodePort: 31575
- name: sqs
enabled: false
endpoint: ""
internalPort: 4576
externalPort: 4576
nodePort: 31576
- name: ssm
enabled: false
internalPort: 4583
externalPort: 4583
nodePort: 31583
- name: dynamodbstreams
enabled: false
internalPort: 4570
externalPort: 4570
nodePort: 31570
- name: firehose
enabled: false
internalPort: 4573
externalPort: 4573
nodePort: 31573
- name: lambda
enabled: false
internalPort: 4574
externalPort: 4574
nodePort: 31574
- name: redshift
enabled: false
internalPort: 4577
externalPort: 4577
nodePort: 31577
- name: es
enabled: false
internalPort: 4578
externalPort: 4578
nodePort: 31578
- name: ses
enabled: false
internalPort: 4579
externalPort: 4579
nodePort: 31579
- name: route53
enabled: false
internalPort: 4580
externalPort: 4580
nodePort: 31580
- name: cloudwatch
enabled: false
internalPort: 4582
externalPort: 4582
nodePort: 31582
- name: secretsmanager
enabled: false
internalPort: 4583
externalPort: 4583
nodePort: 31583
Copy link
Contributor

Choose a reason for hiding this comment

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

After version 0.11.5 of localstack we are using edge port 4566 to access all the services. So we just need to expose port 4566 and 4571. We can remove the rest.

Copy link

@sidhuko sidhuko Dec 6, 2020

Choose a reason for hiding this comment

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

How about setting <SERVICE>_PORT_EXTERNAL or is this not required anymore?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, as per my knowledge it is deprecated. We are using single entrypoint for all the services (by default 4566) and 4571.

Ref. https://github.com/localstack/localstack#announcements
Ref. docker-compose.yaml

Comment on lines +14 to +111
- name: apigateway
enabled: false
endpoint: ""
internalPort: 4567
externalPort: 4567
nodePort: 31567
- name: cloudformation
enabled: false
endpoint: ""
internalPort: 4581
externalPort: 4581
nodePort: 31581
- name: dynamodb
enabled: false
endpoint: ""
internalPort: 4569
externalPort: 4569
nodePort: 31569
- name: elasticsearch
enabled: false
endpoint: ""
internalPort: 4571
externalPort: 4571
nodePort: 31571
- name: kinesis
enabled: false
endpoint: ""
internalPort: 4568
externalPort: 4568
nodePort: 31568
- name: s3
enabled: false
endpoint: ""
internalPort: 4572
externalPort: 4572
nodePort: 31572
- name: sns
enabled: false
endpoint: ""
internalPort: 4575
externalPort: 4575
nodePort: 31575
- name: sqs
enabled: false
endpoint: ""
internalPort: 4576
externalPort: 4576
nodePort: 31576
- name: ssm
enabled: false
internalPort: 4583
externalPort: 4583
nodePort: 31583
- name: dynamodbstreams
enabled: false
internalPort: 4570
externalPort: 4570
nodePort: 31570
- name: firehose
enabled: false
internalPort: 4573
externalPort: 4573
nodePort: 31573
- name: lambda
enabled: false
internalPort: 4574
externalPort: 4574
nodePort: 31574
- name: redshift
enabled: false
internalPort: 4577
externalPort: 4577
nodePort: 31577
- name: es
enabled: false
internalPort: 4578
externalPort: 4578
nodePort: 31578
- name: ses
enabled: false
internalPort: 4579
externalPort: 4579
nodePort: 31579
- name: route53
enabled: false
internalPort: 4580
externalPort: 4580
nodePort: 31580
- name: cloudwatch
enabled: false
internalPort: 4582
externalPort: 4582
nodePort: 31582
- name: secretsmanager
enabled: false
internalPort: 4583
externalPort: 4583
nodePort: 31583
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this except 4566 and 4571.

@sidhuko
Copy link

sidhuko commented Dec 6, 2020

Sorry, been a busy week. I've created a new branch (localstack-chart) to submit as a helm chart on their repository but it needs some work. I am unsure about contributors comments above as it seems we still have <SERVICE>_PORT_EXTERNAL and <SERVICE>_BACKEND. I am not sure if both are still used? I've supported <SERVICE>_BACKEND which can be specified by passing an key value pair under services of the new configuration in new branch. If neither are support or will be deprecated it will make it easier. Otherwise I can still expose multiple ports by reverting to what we have here.

We will also need to expose the new environment variables as configuration in the deployment and values file.

@macnev2013
Copy link
Contributor

Sorry, been a busy week. I've created a new branch (localstack-chart) to submit as a helm chart on their repository but it needs some work. I am unsure about contributors comments above as it seems we still have <SERVICE>_PORT_EXTERNAL and <SERVICE>_BACKEND. I am not sure if both are still used? I've supported <SERVICE>_BACKEND which can be specified by passing an key value pair under services of the new configuration in new branch. If neither are support or will be deprecated it will make it easier. Otherwise I can still expose multiple ports by reverting to what we have here.

Hey @sidhuko, As far as I know we don't use it anymore. @whummer can you confirm it?

@macnev2013
Copy link
Contributor

We will also need to expose the new environment variables as configuration in the deployment and values file.

Yes @sidhuko, we need to add new variables as well. For the starting point maybe we can start importing variables present in the compose file.

Ref. docker-compose.yaml

@mforutan
Copy link
Contributor Author

mforutan commented Dec 7, 2020

@macnev2013 @sidhuko, what do you think if we change the chart's version to 0.1.0-beta.0 and merge this PR to have the process of releasing the chart in place then we can fix the rest of issues and release the chart in next PRs, my aim here is to keep these two steps separated so everyone can contribute individually.

@whummer
Copy link
Member

whummer commented Dec 7, 2020

Thanks @mforutan for creating the initial chart version 👍 and thanks @macnev2013 @sidhuko for reviewing! Agreed that we can merge this initial version as-is, we'll make a couple of changes and enhancements (e.g., removing the legacy service ports) as we go.. Thanks

There are couple of tasks that need to be done before this can be merged:
A gh-pages branch need to be created
GitHub Pages need to be enabled in the setting

Github Pages has been enabled for this repo (currently on master branch) - wondering if we need to move the chart to a localstack.github.io repo to enable hosting of the Githube page..?

@whummer whummer merged commit 4f14113 into localstack:master Dec 7, 2020
@mforutan
Copy link
Contributor Author

mforutan commented Dec 7, 2020

hi @whummer, thank you for merging this PR but it failed to deploy the chart as the gh-pages branch wasn't created, can you please do these steps to get the chart deployed?

@whummer
Copy link
Member

whummer commented Dec 7, 2020

Thanks @mforutan . Actually, I think we may need to move the chart to a separate repo in that case, or do you know if there is a way to prevent the helm/chart-releaser-action@v1.1.0 Github action plugin from creating tags (since we're already using tags to create LocalStack code releases)?

@mforutan
Copy link
Contributor Author

mforutan commented Dec 7, 2020

helm/chart-releaser uses GitHub release to store the chart and also to compare it with the existing versions,

If you are able to create a new repo, I am more than happy to create a new PR against that or we may need to see how to ignore this new tags in code releases.

protocol: TCP
port: {{ $val.externalPort }}
targetPort: {{ $val.internalPort }}
{{- if contains "NodePort" .Values.service.type }}

Choose a reason for hiding this comment

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

This creates a scope error:
Error: template: localstack/templates/service.yaml:28:40: executing "localstack/templates/service.yaml" at <.Values.service.type>: nil pointer evaluating interface {}.service

I think it should be
$.Values.service.type

@sidhuko
Copy link

sidhuko commented Dec 7, 2020 via email

@whummer
Copy link
Member

whummer commented Dec 7, 2020

Thanks all for your inputs - we'll first migrate the chart into a separate repo later today, then we can continue making the incremental changes. Will share the details here shortly. Thanks

whummer pushed a commit that referenced this pull request Dec 7, 2020
@whummer
Copy link
Member

whummer commented Dec 7, 2020

@mforutan @sidhuko @macnev2013 @JamesArthurHolland Moved the chart into this new repo: https://github.com/localstack/helm-charts . We'll create the gh-pages branch and the Github actions build shortly.. Thanks

@mforutan mforutan deleted the add-helm-chart branch December 7, 2020 22:02
@mforutan
Copy link
Contributor Author

mforutan commented Dec 7, 2020

@whummer Awesome!

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.

Official helm chart 🎉
6 participants