Add namespace to wego-app manifests #1041
Conversation
luizbafilho
changed the title
manifests/manifests.go
Outdated
|
|
||
| //go:embed wego-app/deployment.yaml | ||
| var WegoAppDeployment []byte | ||
| // GenerateWegoManifests generates wego-app manifest from a template |
There was a problem hiding this comment.
Nit: manifest => manifests
| //go:embed crds/wego.weave.works_apps.yaml | ||
| AppCRD []byte | ||
| //go:embed wego-app/* | ||
| wegoAppTemplates embed.FS |
There was a problem hiding this comment.
Nice! Cleans things up a lot 👍
manifests/manifests_test.go
Outdated
| Expect(Deployment.Spec.Template.Spec.Containers[0].Image).To(ContainSubstring(v)) | ||
| for _, m := range manifests { | ||
| if strings.Contains(string(m), "kind: Deployment") { | ||
| Expect(string(manifests[0])).To(ContainSubstring("namespace: my-namespace")) |
There was a problem hiding this comment.
This line and the next: manifests[0] => m
| @@ -39,7 +39,7 @@ var _ = Describe("Sync", func() { | |||
| Expect(err.Error()).To(HavePrefix("failed getting application")) | |||
| }) | |||
|
|
|||
| It("sets proper annotation tag to the resource", func() { | |||
| XIt("sets proper annotation tag to the resource", func() { | |||
There was a problem hiding this comment.
Is this a temporary fix until the sync test is working?
pkg/services/gitops/install.go
Outdated
| return nil, fmt.Errorf("could not apply wego-app deployment manifest: %w", err) | ||
| for _, m := range wegoAppManifests { | ||
| if err := g.kube.Apply(ctx, m, params.Namespace); err != nil { | ||
| return nil, fmt.Errorf("error applying wego-app manifest %s: %w", string(m), err) |
There was a problem hiding this comment.
Nit: "string" not needed here
| Expect(err).ShouldNot(HaveOccurred()) | ||
| }) | ||
|
|
||
| installAndVerifyWego(namespace) |
There was a problem hiding this comment.
This will need a URL with the new directory structure changes
There was a problem hiding this comment.
what do you mean?
manifests/manifests.go
Outdated
|
|
||
| //go:embed wego-app/role-binding.yaml | ||
| var WegoAppRoleBinding []byte | ||
|
|
||
| func init() { | ||
| Manifests = [][]byte{ |
There was a problem hiding this comment.
Do we think this will ever have more than one manifest in it any more? If not, maybe this should just be an "AppCRDManifest" variable 🤔
There was a problem hiding this comment.
yep, I'll remove it
pkg/services/gitops/install.go
Outdated
| @@ -74,15 +74,17 @@ func (g *Gitops) Install(params InstallParams) (map[string][]byte, error) { | |||
| version = "latest" | |||
| } | |||
|
|
|||
| wegoAppDeploymentManifest, err := manifests.GenerateWegoAppDeploymentManifest(version) | |||
| wegoAppManifests, err := manifests.GenerateWegoAppManifests(manifests.WegoAppParams{Version: version, Namespace: params.Namespace}) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("error generating wego-app deployment, %w", err) | |||
There was a problem hiding this comment.
Could we update this error message to match the new behavior?
manifests/manifests.go
Outdated
| type deploymentParameters struct { | ||
| Version string | ||
| } | ||
| templates, _ := fs.ReadDir(wegoAppTemplates, wegoManifestsDir) |
There was a problem hiding this comment.
should we handle error here?
There was a problem hiding this comment.
it's very unlikely this will ever return any error because it's reading directly from memory, but it doesn't hurt adding the err check
manifests/manifests.go
Outdated
| ) | ||
|
|
||
| var Manifests [][]byte | ||
| var ( | ||
| Manifests [][]byte |
There was a problem hiding this comment.
It seems like we can remove this array now? It is being filled out in init but only with the AppCRD which is already public, so then we can access AppCRD directly instead?
Closes: #988 #963
What changed?
namespaceto thewego-appmanifests we deploy since that's a requirement from flux 0.18.0Why?
How did you test it?
Release notes
Documentation Changes