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

[#1693] Move defaults to airy yaml #1700

Merged
merged 10 commits into from
May 21, 2021
Merged

Conversation

chrismatix
Copy link
Contributor

resolves #1693

return color.Colorize(color.Cyan, "#\t"+input)
})
provider := providers.MustGet(providers.ProviderName(providerName), w)
overrides := provider.GetOverrides()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of passing the overrides as --set flags we merge them into the airy.yaml first before using it as a values file in Helm.

Copy link
Contributor

Choose a reason for hiding this comment

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

I created a cluster and AWS and the annotation for the loadbalancer was not picked-up (classical loadbalancer was created).

}

//go:embed src
var templateDir embed.FS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored the templating part of the workspace module into its own module

if err != nil {
return err
}
if _, err := os.Stat(dstPath); err == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bug: The create command was previously overwriting config that was already present.

@@ -6,6 +6,9 @@ go 1.16
// Automatically generated by running //tools/update-deps

require (
github.com/Masterminds/goutils v1.1.1 // indirect
github.com/Masterminds/semver v1.5.0 // indirect
github.com/Masterminds/sprig v2.22.0+incompatible // indirect
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same templating library that helm uses

data:
HOST: {{ .Values.global.host }}
HOST: {{ .Values.global.kubernetes.host }}
---
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hoisted the security configmap to the top level

@chrismatix chrismatix marked this pull request as ready for review May 7, 2021 10:07
@chrismatix chrismatix requested a review from lucapette as a code owner May 7, 2021 10:07
Copy link
Contributor

@ljupcovangelski ljupcovangelski left a comment

Choose a reason for hiding this comment

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

I think that the order in which we generate the airy.yaml file should be:

  • we grab the helm defaults
  • we grab the overrides for a specific provider
  • we provision Airy core in the Kubernetes cluster add grab the generated values (such as host)
  • we generate the airy.yaml file at the end

return color.Colorize(color.Cyan, "#\t"+input)
})
provider := providers.MustGet(providers.ProviderName(providerName), w)
overrides := provider.GetOverrides()
Copy link
Contributor

Choose a reason for hiding this comment

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

I created a cluster and AWS and the annotation for the loadbalancer was not picked-up (classical loadbalancer was created).

cli/pkg/workspace/template/src/airy.yaml Show resolved Hide resolved
@chrismatix
Copy link
Contributor Author

In an offline conversation @ljupcovangelski and I decided that the best move forward would be to:

  1. default the host value in the helm values.yaml with http://airy.core
  2. leave the host variable empty in the airy.yaml for aws
  3. use the AWS PostInstallation hook to update the airy.yaml with the correct host value

To improve 1. in such a way that we don't even need to default to an imaginary host value we introduce this follow up ticket for improvement #1761

Copy link
Contributor

@ljupcovangelski ljupcovangelski left a comment

Choose a reason for hiding this comment

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

I tested and it works great 🎉

@chrismatix chrismatix changed the title Move defaults to airy yaml [#1693] Move defaults to airy yaml May 21, 2021
@chrismatix chrismatix merged commit 14c866b into develop May 21, 2021
@chrismatix chrismatix deleted the feat/1693-template-airy-yaml branch May 21, 2021 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move default values from Helm to the airy yaml
2 participants