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

Simplify Helm install #492

Merged
merged 24 commits into from
Oct 21, 2021
Merged

Simplify Helm install #492

merged 24 commits into from
Oct 21, 2021

Conversation

mxyng
Copy link
Collaborator

@mxyng mxyng commented Oct 19, 2021

Two paradigm shifts are captured in this PR.

  1. Auto-discover registry. If a registry host is not provided, the engine will try to query Kubernetes Services to find an appropriate host. This will use the new label infrahq.com/flavor to find a registry service.

  2. Both the registry and engine has been updated to accept URIs for secrets, right now root API key and engine API key. The services themselves will resolve this reference to actual values. Implementation will vary.
    For now, the only support format is file:// which will look for the secrets in the filesystem at the path provided. In the future, this can be used to get secrets from Vault, AWS KMS, whatever else.

These changes combined allows a fully functional Infra deploy using a single infra Helm chart.

TODO:

  • Global variable overrides for things like image.tag, image.pullPolicy, etc.
  • Fill out README.md and linked docs
  • Update NOTES.txt

Fixes #465
Fixes #484

README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,44 @@
## Introduction
Copy link
Contributor

Choose a reason for hiding this comment

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

Love that you created the first of these!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mxyng mxyng force-pushed the simplify-helm-install branch 4 times, most recently from 4c0c147 to dd8d861 Compare October 20, 2021 02:52
README.md Outdated Show resolved Hide resolved
README.md Outdated
- kind: role
name: viewer
destinations:
- name: <cluster name>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can add a comment here to get the reader context around what the cluster name is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's initially why it set a static cluster name ("my-first-cluster"). I'm happy to add something if you have any suggestions

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the name being left out like this for clarity. What do you think of something like this?

destinations:
          - name: <cluster name> # the cluster name can be set on deploying a new engine, otherwise it will default to the name of the cluster that the engine is running in

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/destinations/kubernetes.md Outdated Show resolved Hide resolved
helm/charts/engine/templates/secret.yaml Show resolved Hide resolved
helm/charts/registry/templates/deployment.yaml Outdated Show resolved Hide resolved
internal/registry/registry.go Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@BruceMacD
Copy link
Collaborator

Are the empty helm/charts/infra/templates/_helpers.tpl and helm/charts/infra/values.yaml intentional?

@@ -0,0 +1,34 @@
*************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work on this!

internal/cmd/cmd.go Outdated Show resolved Hide resolved
@mxyng
Copy link
Collaborator Author

mxyng commented Oct 20, 2021

Are the empty helm/charts/infra/templates/_helpers.tpl and helm/charts/infra/values.yaml intentional?

no, they originally had content but it's no longer necessary

@mxyng mxyng force-pushed the simplify-helm-install branch 2 times, most recently from aaad674 to 3e6d8ab Compare October 20, 2021 22:40
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mxyng mxyng force-pushed the simplify-helm-install branch 2 times, most recently from 8bf8a17 to ac786a6 Compare October 21, 2021 18:02
@infrahq infrahq deleted a comment from github-actions bot Oct 21, 2021
@infrahq infrahq deleted a comment from github-actions bot Oct 21, 2021
@mxyng mxyng force-pushed the simplify-helm-install branch 5 times, most recently from 24b89e1 to e96ca51 Compare October 21, 2021 19:31
README.md Outdated
* [Destinations](./docs/destinations)
* [Kubernetes](./docs/destinations/kubernetes.md)

### Updating Infra
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit/optional thing: should we call this "Upgrading?"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switched to Upgrade instead since there's no other -ing in this doc

@mxyng mxyng force-pushed the simplify-helm-install branch 2 times, most recently from 7677e9e to 5ab61b3 Compare October 21, 2021 20:24
Copy link
Contributor

@jmorganca jmorganca left a comment

Choose a reason for hiding this comment

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

LGTM – incredible work!

@mxyng mxyng force-pushed the simplify-helm-install branch 3 times, most recently from ca66a98 to c340cc8 Compare October 21, 2021 20:37
@mxyng mxyng merged commit 4002fc2 into main Oct 21, 2021
@mxyng mxyng deleted the simplify-helm-install branch October 21, 2021 20:42
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.

Engine API Key gets reset when helm chart is redeployed Simplify infra install process
5 participants