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

initial directory reorg #26

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

initial directory reorg #26

wants to merge 17 commits into from

Conversation

drohnow
Copy link

@drohnow drohnow commented Jul 14, 2023

Proposed directory outlay per shortcut 42812.

Copy link
Member

@chris-sanders chris-sanders left a comment

Choose a reason for hiding this comment

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

Do not check binary files into git

charts/replicated-library-0.13.3.tgz Outdated Show resolved Hide resolved
Copy link
Member

@chris-sanders chris-sanders left a comment

Choose a reason for hiding this comment

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

It doesn't look like the README.md in the root of the repository has been updated. I think that will need to be overhauled fairly significantly.

  • It should tell people to edit the Chart.yaml to match their desired application name and repository
  • It should be sure to mention and link to the platform-examples repository to get people started.
  • I would avoid screenshots of Vendor Portal, they're just going to fall out of date. The Github / GitLab actions and upstream documentation should cover that.
  • Maybe add a section for documenting their high level architecture to encourage people to do that for supportability.

values.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
values.yaml Show resolved Hide resolved
Chart.lock Outdated Show resolved Hide resolved
Chart.yaml Outdated Show resolved Hide resolved
Chart.yaml Outdated Show resolved Hide resolved
Copy link
Member

@chris-sanders chris-sanders left a comment

Choose a reason for hiding this comment

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

A few comments and questions on the readme

README.md Outdated
@@ -3,7 +3,7 @@ Replicated Helm Starter

Example project showcasing how power users can leverage the Replicated CLI Tools to manage a helm chart that can be delivered via Replciated.

This helm version of https://github.com/replicatedhq/replciated-starter-kots adds replicated manifests directly in a helm chart's working directory. To release:
This helm version of https://github.com/replicatedhq/replicated-starter-kots adds replicated manifests directly in a helm chart's working directory. To release:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any reason to refer to the old repo. A new onboarding customer simply doesn't need this information it just muddies things.

README.md Outdated
@@ -3,7 +3,7 @@ Replicated Helm Starter

Example project showcasing how power users can leverage the Replicated CLI Tools to manage a helm chart that can be delivered via Replciated.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't what this is anymore. The examples are in the other repository, this is to set you up with a repository configured to get started packaging quickly.

README.md Outdated
@@ -3,7 +3,7 @@ Replicated Helm Starter

Example project showcasing how power users can leverage the Replicated CLI Tools to manage a helm chart that can be delivered via Replciated.

This helm version of https://github.com/replicatedhq/replciated-starter-kots adds replicated manifests directly in a helm chart's working directory. To release:
This helm version of https://github.com/replicatedhq/replicated-starter-kots adds replicated manifests directly in a helm chart's working directory. To release:

```
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to be talking about releasing, I think it should go at the end not the beginning of the document. If I read this, my very first thought is I need to release something.

README.md Outdated
@@ -14,6 +14,7 @@ replicated release create --auto -y

## Get started

If you are new to writing Helm charts, check out examples of Helm chart written utilizing the Replicated Helm Library chart [here](https://github.com/replicatedhq/platform-examples)

### Copy and Clone

Copy link
Member

Choose a reason for hiding this comment

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

(I can't comment lower down look a few lines below).

I think the line:
You should use the template to create a new **private** repo in your org, for example mycompany/replicated-helm or mycompany/replicated-starter-helm.

Should be removed. I don't know why we would recommend our customers do that. Let's target the whole readme to a new customer who will see this in their collab repo.

README.md Outdated
Comment on lines 36 to 37


Copy link
Member

Choose a reason for hiding this comment

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

We go from "Edit the chart" to "Install the cli".
What we've really done here is transition from writing a chart to releasing one. They aren't ready to release after editing chart.yaml.

Let's add some instructions here, just think about when you write a new chart roughly what's involved.

  • Review your application and gather the information about what you are deploying
  • Check the examples for patterns you might want to implement
  • Create the values.yaml and any accompanying template files to define
  • Test locally with helm template, repeate

Then talk about releasing and make it clear that's what is happening. Probably a good idea to reference the patterns to point out you can have it done via Github/GitLab if you like.

README.md Outdated
- appVersion
- home

note: see Chart.yaml for examples


### 1. Install CLI
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to keep the manual install instructions here in the Readme? Maybe it's a good thing to just link out to?

README.md Outdated
```

You can ensure this is working with
Once the environment variables are set, you can validate configuration by running the following command:

```
replicated release ls
Copy link
Member

Choose a reason for hiding this comment

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

This whole section about manifests and linting I'm not 100% sure is accurate for helm. This reads like it's talking about KOTS (with all the references to 'manifests' instead of talking about charts).

Have you run through this and is it for a native helm v2 install?

README.md Outdated
@@ -158,6 +155,9 @@ There is a file `kurl-installer.yaml` that can be used to manage [kurl.sh](https
replicated installer create --auto
Copy link
Member

Choose a reason for hiding this comment

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

From above: 1. Remove the templates and values from this chart, and just use this repo as an umbrella chart that includes your main chart as a helm dependencyinChart.yaml(runhelm dependency updatebeforehelm package . -d manifests/)

I thought this new version wouldn't require removing templates and values from the chart. does this need updating too?

@drohnow drohnow requested a review from chris-sanders August 1, 2023 21:41
Copy link
Member

@chris-sanders chris-sanders left a comment

Choose a reason for hiding this comment

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

Just a couple of comments I like how this is shaping up.

Comment on lines +20 to +24
# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application. Versions are not expected to
# follow Semantic Versioning. They should reflect the version the application is using.
# It is recommended to use it with quotes.
appVersion: "3.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is an optional field, I'm not a fan and don't personally use it on my own charts. Should we drop this for simplicity?

3. Edit Chart.yaml:
- name: <application_name>
- description: <application_description>
- appVersion: <application_version>
Copy link
Member

Choose a reason for hiding this comment

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

Based on above remove here as well

Comment on lines +5 to +14
# -- Set additional global labels.
labels: {}
# -- Set additional global annotations.
annotations: {}
fullNameOverride: "-"

apps:
vaultwarden:
enabled: true
type: deployment
replicas: 1
initContainers:
copy-test:
image:
repository: busybox
tag: latest
command: ['sh', '-c', 'echo "test" > /data/test.txt']
volumeMounts:
- mountPath: /data
name: vaultwarden
containers:
vaultwarden:
image:
repository: vaultwarden/server
tag: 1.27.0-alpine
volumeMounts:
- mountPath: /data
name: vaultwarden
ports:
- name: http
containerPort: 80
envFrom:
- configMapRef:
name: vaultwarden
volumes:
- name: vaultwarden
persistentVolumeClaim:
claimName: vaultwarden

services:
vaultwarden:
enabled: true
appName: ["vaultwarden"]
type: ClusterIP
ports:
http:
enabled: true
port: 8080
protocol: HTTP
targetPort: 80

ingresses:
vaultwarden:
enabled: true
serviceName: vaultwarden
hosts:
- host: vaultwarden.example.com
paths:
- path: /
pathType: Prefix
service:
port: 8080
tls:
- hosts:
- vaultwarden.example.com
secretName: vaultwarden-tls-secret

configmaps:
vaultwarden:
enabled: true
data:
SOME_VAR: "some-value"
# -- Set the full object prefix, defaults to releasName-ChartName if not set. This value takes precedence over nameOverride.
# Set to "-" to disable object name prefixing.
fullNameOverride:
# -- Set an override for the ChartName, defaults to ChartName if not set.
nameOverride:
# -- When `true``, the feature to automatically restarts an App pods when a ConfigMap or Secret changes is enabled.
Copy link
Member

Choose a reason for hiding this comment

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

I think I would only set the appReload here. The other values can be used if needed, although I could also see someone setting fullNameOverride: "-" as a default based on the likelihood that most vendors won't be installing multiple times on the same chart.

Comment on lines -19 to +32

This repo is a [GitHub Template Repository](https://help.github.com/en/articles/creating-a-repository-from-a-template). You can create a private copy by using the "Use this Template" link in the repo:

![Template Repo](https://docs.github.com/assets/cb-95207/mw-1000/images/help/repository/use-this-template-button.webp)

You should use the template to create a new **private** repo in your org, for example `mycompany/replicated-helm` or `mycompany/replicated-starter-helm`.

Once you've created a repository from the template, you'll want to `git clone` your new repo and `cd` into it locally.



### 1. Install CLI

To start, you'll want to install the `replicated` CLI.
You can install with [homebrew](https://brew.sh) or grab the latest Linux or macOS version from [the replicatedhq/replicated releases page](https://github.com/replicatedhq/replicated/releases).

#### Brew

```shell script
brew install replicatedhq/replicated/cli
```

#### Manual

```shell script
curl -s https://api.github.com/repos/replicatedhq/replicated/releases/latest \
| grep "browser_download_url.*$(uname | tr '[:upper:]' '[:lower:]')_amd64.tar.gz" \
| cut -d : -f 2,3 \
| tr -d \" \
| cat <( echo -n "url") - \
| curl -fsSL -K- \
| tar xvz replicated
```
Then move `./replicated` to somewhere in your `PATH`:


```shell script
mv replicated /usr/local/bin/
```

#### Verifying

You can verify it's installed with `replicated version`:

```text
$ replicated version
```
```json
{
"version": "0.31.0",
"git": "c67210a",
"buildTime": "2020-09-03T18:31:11Z",
"go": {
"version": "go1.14.7",
"compiler": "gc",
"os": "darwin",
"arch": "amd64"
}
}
```


### Configure environment

You'll need to set up two environment variables to interact with vendor.replicated.com:

```
export REPLICATED_APP=...
export REPLICATED_API_TOKEN=...
```

`REPLICATED_APP` should be set to the app slug from the Settings page:

<p align="center"><img src="./doc/REPLICATED_APP.png" width=600></img></p>
5. Install Replicated CLI - [Instructions](https://docs.replicated.com/reference/replicated-cli-installing#install-the-replicated-cli)

Next, create a Service Account API token from the vendor portal under [Service Accounts](https://vendor.replicated.com/team/serviceaccounts):
6. Configure environment variables -- [Instructions](https://docs.replicated.com/reference/replicated-cli-installing#set-environment-variables)
Copy link
Member

Choose a reason for hiding this comment

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

Part of me says move this down to a 'releasing' section. Let people get started with helm w/o these steps since they won't use them until they have done local testing. In fact, it might make since to never even tell them to do this and just tell them to enable the github actions once they are ready to start pushing to their development channel.

If they don't have to go to Vendor Portal at all to get started, maybe they'll get started and get to their first chart test cycle faster. That's what I'm looking to optimize here, get them through a "helm template, change something, helm template" cycle as soon as possible.

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.

3 participants