Fluxcd fixes #814

zjaco13 · 2023-08-08T17:37:18Z

Issue #, if available:

Description of changes:
Added a new fluxcd repository object, and array of those objects to fluxcd props to allow for multiple repositories and multiple paths.

Changes are breaking changes

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

shapirov103

Overall, refactoring makes sense and there should not be first/second class citizen between bootstrap and workload repos - that consistency improves the design.
However, I have a couple of comments on the specific design choices for the structure, please take a look.

shapirov103 · 2023-08-08T17:45:11Z

lib/addons/fluxcd/index.ts

-  */
-  version?: string;
-
+export interface FluxCDGitRepo {


It should derive from GitopsApplicationDeployment and add fields that are flux specific.

shapirov103 · 2023-08-08T17:46:12Z

lib/addons/fluxcd/index.ts

-  */
-  version?: string;
-
+export interface FluxCDGitRepo {


Why is bootstrapRepository optional here? Does this structure make sense without bootstrap repo? See my other comment that it should really extend GitOpsApplicationDeployment

shapirov103 · 2023-08-08T17:47:46Z

lib/addons/fluxcd/index.ts

@@ -61,7 +39,7 @@ export interface FluxCDAddOnProps extends HelmAddOnUserProps {

  /** 
  * Flux Kustomization Target Namespace.
-  * Note: when set, this parameter will override all the objects that are part of the Kustomization, please see https://fluxcd.io/flux/components/kustomize/kustomization/#target-namespace */
+  * Default `default` */
  fluxTargetNamespace?: string;


is this namespace ever different for these apps? my assumption is that this is the namespace where the GitRepository CR will be created, please confirm. If my assumption is correct, this namespace is going to be the same for all of them.
If it is something else let's discuss.

The target namespace is where we want the kustomizations to be deployed. This can be specified per kustomization, whereas the gitrepositories are deployed in the same namespace that flux is deployed

The name doesn't convey it, should be fixed, e.g. kustomizationNamespace or kustomizationTargetNamespace.
If there is a particular set of attributes specific to kustomizations then it would make more sense to have an interface for Kustomization with path, namespace, etc.

is this namespace ever different for these apps? my assumption is that this is the namespace where the GitRepository CR will be created, please confirm. If my assumption is correct, this namespace is going to be the same for all of them. If it is something else let's discuss.

I think we might want NOT to make the assumption of a single namespace. E.g. in this case https://github.com/aws-observability/aws-observability-accelerator/blob/main/artifacts/grafana-operator-manifests/eks/infrastructure/amg_grafana-amp-datasource.yaml we use grafana-operator as a namespace, but we might want to add another path, with other manifests related to another operator and so likely another namespace.
I agree on the name. The parameter is finally going to be applied to https://fluxcd.io/flux/components/kustomize/kustomization/#target-namespace and like mentioned elsewhere, it should be optional, to avoid forcing override.

elamaran11

Few minor comments.

elamaran11 · 2023-08-09T01:34:02Z

lib/addons/fluxcd/index.ts


+    /** 


Indentation issues and the comment is incomplete.

elamaran11 · 2023-08-09T01:34:09Z

lib/addons/fluxcd/index.ts

-  * Flux Kustomization Prune.
-  * Default `true` */
-  fluxPrune?: boolean;
+    /**


Indentation issues and the comment is incomplete.

freschri · 2023-08-09T14:48:31Z

Please @zjaco13 confirm the changes have been tested with https://github.com/aws-observability/cdk-aws-observability-accelerator/tree/main/lib/single-new-eks-opensource-observability-construct
thanks

freschri · 2023-08-09T15:20:52Z

lib/addons/fluxcd/index.ts

+    kustomizations?: FluxKustomizationProps[];
+}
+
+export interface FluxKustomizationProps {


would it make sense to move this interface and the repo interface to the related files?

I clearly remember replying to this, but my comment does not show up. We keep contracts together as customers will need all of them exported. so rather than creating dependencies between files, i suggest that we keep all contracts related to the addon either in the index or in a separate file called contracts.ts, types.ts and such.

freschri · 2023-08-09T15:25:20Z

lib/addons/fluxcd/index.ts

+/**
+ * Options for a FluxCD repository
+ */
+export interface FluxCDGitRepo extends Omit<spi.GitOpsApplicationDeployment, "repository">{


we probably need to review namings and make consistent choices, e.g. flux vs fluxcd. also do we want the flux prefix on kustomizations? then use everywhere... e.g. I see kustomizations and fluxKustomizationPath and fluxPrune in Kustomization props...

Flux in the name of the interface is enough to remove the prefix from each field. The interfaces should be publicly exported, and if they are part of the addon contract, they should stay with the add-on.

freschri · 2023-08-09T15:27:31Z

lib/addons/fluxcd/gitrepository.ts

@@ -6,23 +6,23 @@ import { setPath } from "../../utils";
 */
 export class FluxGitRepository {

-    constructor(private readonly bootstrapRepo: spi.GitOpsApplicationDeployment) {}
+    constructor(private readonly bootstrapRepo: spi.ApplicationRepository) {}


maybe bootstrap in the name can be removed now?

freschri · 2023-08-09T15:38:15Z

lib/addons/fluxcd/gitrepository.ts


-    public generate(namespace: string, fluxSyncInterval: string, fluxSecretRefName: string) {
+    public generate(name: string, namespace: string, fluxSyncInterval: string, fluxSecretRefName: string) {

        const repository = this.bootstrapRepo!;


do we need the exclamation mark here? it is not an optional...

freschri · 2023-08-09T15:54:26Z

lib/addons/fluxcd/index.ts

+    * Flux Kustomization Prune.
+    * Default `true` */
+    fluxPrune?: boolean;
+


there should be another interval property here for Kustomization. it is different than GitRepository.

freschri · 2023-08-09T15:55:11Z

lib/addons/fluxcd/index.ts

+    const manifest =fluxKustomization.generate(
+      fluxcdGitRepo.name + "-" + index,
+      fluxcdGitRepo.namespace ?? namespace,
+      fluxcdGitRepo.fluxSyncInterval!,


need to check we are using the kustomization interval and not the repo interval

I agree to this comment but generally speaking the intervals are coded the same across GitRepository and Kustomization

freschri · 2023-08-09T15:57:14Z

lib/addons/fluxcd/kustomization.ts


-    public generate(name: string, namespace: string, fluxSyncInterval: string,  fluxPrune: boolean, fluxTimeout: string, bootstrapValues: spi.Values, fluxKustomizationPath: string, fluxTargetNamespace?: string) {
+    public generate(name: string, namespace: string, fluxSyncInterval: string, fluxPrune: boolean, fluxTimeout: string, bootstrapValues: spi.Values, fluxKustomizationPath: string, fluxTargetNamespace?: string) {


I think we should aim for a generate() without params, all should be already in self.
also, would it be a better naming generateManifest?
same considerations for gitrepository

elamaran11

Minor comment.

elamaran11 · 2023-08-09T21:21:13Z

lib/addons/fluxcd/index.ts

+    const manifest =fluxKustomization.generate(
+      fluxcdGitRepo.name + "-" + index,
+      fluxcdGitRepo.namespace ?? namespace,
+      fluxcdGitRepo.fluxSyncInterval!,


I agree to this comment but generally speaking the intervals are coded the same across GitRepository and Kustomization

elamaran11 · 2023-08-09T21:24:19Z

docs/addons/fluxcd.md

+                targetRevision: "master",
+            },
+            values: {
+                "region": "us-east-2"


Region can be consistent.

zjaco13 · 2023-08-09T23:07:28Z

Please @zjaco13 confirm the changes have been tested with https://github.com/aws-observability/cdk-aws-observability-accelerator/tree/main/lib/single-new-eks-opensource-observability-construct thanks

@freschri can confirm it works

shapirov103

LGTM, we will need a PR to the main branch

elamaran11 · 2023-08-10T20:08:06Z

@zjaco13 Please PR to main

zjaco13 added 2 commits August 8, 2023 10:34

add flux repo array and update docs

cad0e5f

revert changes to this file

5f88c1c

zjaco13 requested review from aws-ia-ci, parkand1, shapirov103, youngjeong46, elamaran11 and a team as code owners August 8, 2023 17:37

shapirov103 reviewed Aug 8, 2023

View reviewed changes

zjaco13 added 2 commits August 8, 2023 16:18

make new interfaces and fix docs

9ba3581

fix gitrepo name

203999d

elamaran11 reviewed Aug 9, 2023

View reviewed changes

freschri reviewed Aug 9, 2023

View reviewed changes

elamaran11 reviewed Aug 9, 2023

View reviewed changes

zjaco13 added 2 commits August 9, 2023 15:08

fix naming issues

9ab0bfb

version consitency

b074e79

shapirov103 approved these changes Aug 9, 2023

View reviewed changes

elamaran11 merged commit 05cb2b3 into aws-quickstart:809-fluxcd-fails-to-deploy-multiple-kustomizations-following-pr-794 Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fluxcd fixes #814

Fluxcd fixes #814

zjaco13 commented Aug 8, 2023 •

edited

Loading

shapirov103 left a comment

shapirov103 Aug 8, 2023

shapirov103 Aug 8, 2023

shapirov103 Aug 8, 2023

zjaco13 Aug 8, 2023 •

edited

Loading

shapirov103 Aug 8, 2023

freschri Aug 9, 2023

elamaran11 left a comment

elamaran11 Aug 9, 2023

elamaran11 Aug 9, 2023

freschri commented Aug 9, 2023

freschri Aug 9, 2023

shapirov103 Aug 9, 2023

freschri Aug 9, 2023

shapirov103 Aug 9, 2023

freschri Aug 9, 2023

freschri Aug 9, 2023

freschri Aug 9, 2023

freschri Aug 9, 2023

elamaran11 Aug 9, 2023

freschri Aug 9, 2023 •

edited

Loading

elamaran11 left a comment

elamaran11 Aug 9, 2023

elamaran11 Aug 9, 2023

zjaco13 commented Aug 9, 2023

shapirov103 left a comment

elamaran11 commented Aug 10, 2023


		public generate(name: string, namespace: string, fluxSyncInterval: string, fluxPrune: boolean, fluxTimeout: string, bootstrapValues: spi.Values, fluxKustomizationPath: string, fluxTargetNamespace?: string) {
		public generate(name: string, namespace: string, fluxSyncInterval: string, fluxPrune: boolean, fluxTimeout: string, bootstrapValues: spi.Values, fluxKustomizationPath: string, fluxTargetNamespace?: string) {

Fluxcd fixes #814

Fluxcd fixes #814

Conversation

zjaco13 commented Aug 8, 2023 • edited Loading

shapirov103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjaco13 Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elamaran11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freschri commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freschri Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

elamaran11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjaco13 commented Aug 9, 2023

shapirov103 left a comment

Choose a reason for hiding this comment

elamaran11 commented Aug 10, 2023

zjaco13 commented Aug 8, 2023 •

edited

Loading

zjaco13 Aug 8, 2023 •

edited

Loading

freschri Aug 9, 2023 •

edited

Loading