#322 Set config store to be included by default #329

carter-cundiff · 2024-09-12T15:46:08Z

Update documentation to live under supporting components

docs/modules/ROOT/pages/configuration-store.adoc

ewilkins-csi · 2024-09-12T16:08:51Z

docs/modules/ROOT/pages/configuration-store.adoc

+This file defines two properties, `connector` and `topic`, each with the group name `messaging`.
+
+===== Environment Specific Overrides
+In addition to the base properties file introduced above, one can optionally specify a secondary properties file that houses environment


S: Again I think we can largely lean on Krausening docs here and only call out the aiSSEMBLE-specific stuff.

Looking at the krausening related section for this, while it does explain how the extension works, I think it introduces too many unrelated details. Specifically the stuff the env var KRAUSENING_EXTENSIONS and the code block for retrieving the properties.

Would rather just keep a straightforward example here that just focus on the properties files.

docs/modules/ROOT/pages/configuration-store.adoc

ewilkins-csi · 2024-09-12T16:23:08Z

docs/modules/ROOT/pages/configuration-store.adoc

+
+==== Mounting Properties
+In order for the store to read the configurations, the properties files will need to be placed into a https://kubernetes.io/docs/concepts/storage/persistent-volumes/#lifecycle-of-a-volume-and-claimpersistent[Persistent Volume,role=external,window=_blank] (PV)
+mounted to the configuration store kubernetes resource. It is assumed a PV will meet most projects needs and is the most widely accepted 


S: We've got a fine line to walk here between convenience/comprehensiveness in the antora docs, and confining chart-specific details to the chart documentation. I think what we have is a pretty good split, but I think we could pull it up even a little higher here. For instance instead of getting into "Persistent Volumes" specifically, we can just say the high-level goal: Attaching the properties to the local file system of the service through a mounted volume. I don't have great specifics here, but maybe the key question is: Could someone with very little Kubernetes/Helm knowledge read the paragraph and get good understanding of what's happening and why (even if they don't fully understand the "how").

Echoing the above, whether right or wrong, a takeaway I could have from this is that a persistent volume is the only option. I think I agree that it would be better to pull this up slightly.

Brought up to higher level by removing kubernetes specific wording.

ewilkins-csi · 2024-09-12T16:25:03Z

docs/modules/ROOT/pages/configuration-store.adoc

+[source,yaml]
+----
+  env:
+    BASE_PROPERTY: <URI housing base property configurations>


S: This is an example of the type of detail that I think is may be too much for these docs. There are two risks with this example: 1) it muddies the waters of the overall explanation of the feature and generally how it works, and 2) it's very likely to fall out of date as the chart changes.

S: I found this section to be a bit confusing. To be fair, I am not sure how much of the "how" a developer needs/wants to know in order to configure these things appropriately for their project. My question for this section is what do we really want the reader to take away from it? If you were to list out those 3-4 high level bullets, maybe that would help make this more succinct?

Moved technical portion to helm chart README

docs/modules/ROOT/pages/configuration-store.adoc

nartieri

Added some substantive and administrative feedback. Happy to discuss as needed.

nartieri · 2024-09-12T17:40:56Z

docs/modules/ROOT/pages/configuration-store.adoc

+In addition to the base properties file introduced above, one can optionally specify a secondary properties file that houses environment
+specific configurations. These configurations will then used to override their respective base configurations.
+
+Continuing with the example above, suppose one defines an additional properties file named `messaging.properties` like the following:


?: Would it be common to have to properties files (a base and an environment specific properties file) with the same name? I guess it is a bit confusing as it feels like it would make things more difficult to manage.

In theory they would need to be the same name but in different directories for the environment ovverride to work. This is no longer applicable though as I removed the property file names from this section to make it less confusing.

nartieri · 2024-09-12T17:43:17Z

docs/modules/ROOT/pages/configuration-store.adoc

+
+==== Mounting Properties
+In order for the store to read the configurations, the properties files will need to be placed into a https://kubernetes.io/docs/concepts/storage/persistent-volumes/#lifecycle-of-a-volume-and-claimpersistent[Persistent Volume,role=external,window=_blank] (PV)
+mounted to the configuration store kubernetes resource. It is assumed a PV will meet most projects needs and is the most widely accepted 


A: If the existing text remains I would reword to "...a PV will meet the needs of most projects and is the..."

Text removed.

nartieri · 2024-09-12T17:51:53Z

docs/modules/ROOT/pages/configuration-store.adoc

+
+==== Mounting Properties
+In order for the store to read the configurations, the properties files will need to be placed into a https://kubernetes.io/docs/concepts/storage/persistent-volumes/#lifecycle-of-a-volume-and-claimpersistent[Persistent Volume,role=external,window=_blank] (PV)
+mounted to the configuration store kubernetes resource. It is assumed a PV will meet most projects needs and is the most widely accepted 


Echoing the above, whether right or wrong, a takeaway I could have from this is that a persistent volume is the only option. I think I agree that it would be better to pull this up slightly.

nartieri · 2024-09-12T17:55:53Z

docs/modules/ROOT/pages/configuration-store.adoc

+[source,yaml]
+----
+  env:
+    BASE_PROPERTY: <URI housing base property configurations>


S: I found this section to be a bit confusing. To be fair, I am not sure how much of the "how" a developer needs/wants to know in order to configure these things appropriately for their project. My question for this section is what do we really want the reader to take away from it? If you were to list out those 3-4 high level bullets, maybe that would help make this more succinct?

nartieri · 2024-09-12T18:06:11Z

docs/modules/ROOT/pages/configuration-store.adoc

+} 
+----
+
+NOTE: The name of the configuration service may vary depending on the projects helm chart configuration, by default it is


A: missing apostrophe - project's

nartieri · 2024-09-12T18:07:18Z

docs/modules/ROOT/pages/configuration-store.adoc

+NOTE: The name of the configuration service may vary depending on the projects helm chart configuration, by default it is
+`http://configuration-store.config-store-ns.svc:8083`
+
+==== Resource Injection


A: Should this be Kubernetes Resource Injection to match what we called it above?

nartieri · 2024-09-12T18:15:12Z

docs/modules/ROOT/pages/configuration-store.adoc

+
+==== Resource Injection
+The configuration store provisions a https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/[kubernetes admission webhook,role=external,window=_blank] as part of its deployment. This webhook is used for injecting property values
+from the configuration store directly into kubernetes resources as they deployed to the cluster.


A: recommend minor edit to "...as they are deployed..."

?: Something that struck me as I was reading through this page is we give examples of using what we have built, but don't necessarily give examples of when to use them, or why you would choose one approach over another. You can tell me to pound sand if that is not something we typically do and is why we did not do it here. But I do wonder if providing some help to the user would be useful. E.g., when would they want to use the REST endpoint/API vice Kubernetes Resource Injection; when should they use Krausening vs Vault, and maybe some others. Anyway, it is just a bit of food for thought.

The backend storage section gives a little insight into why you would want to use the different options.

In terms of the REST Endpoint vs kubernetes injection, I think the injection is the preferred method and can be used in replacement of the REST Endpoint in most use cases. However, at the end of the day it really comes down to user preference if they'd rather use the endpoint.

docs/modules/ROOT/pages/configuration-store.adoc

ewilkins-csi · 2024-09-13T16:23:34Z

docs/modules/ROOT/pages/configuration-store.adoc

+NOTE: The `topic` property is overwritten when the environment specific value `ciTopic`.
+
+==== Deploying the Configuration Store
+The https://github.com/boozallen/aissemble/blob/{git-tree}/extensions/extensions-helm/aissemble-configuration-store-chart/README.md[configuration store helm chart,role=external,window=_blank] is used for easily deploying it to a Kubernetes cluster. As part of the deployment, one must


Suggested change

The https://github.com/boozallen/aissemble/blob/{git-tree}/extensions/extensions-helm/aissemble-configuration-store-chart/README.md[configuration store helm chart,role=external,window=_blank] is used for easily deploying it to a Kubernetes cluster. As part of the deployment, one must

The https://github.com/boozallen/aissemble/blob/{git-tree}/extensions/extensions-helm/aissemble-configuration-store-chart/README.md[Configuration Store Helm chart,role=external,window=_blank] can easily deploy the store to a Kubernetes cluster. As part of the deployment, one must

Not sure if we're capitalizing "Configuration Store". I'm fine with either, but Helm should be capitalized.

Capitalized all helms and config stores

docs/modules/ROOT/pages/configuration-store.adoc

carter-cundiff · 2024-09-16T15:45:57Z

extensions/extensions-helm/aissemble-jenkins-chart/values.yaml

@@ -1,10 +0,0 @@
-# Default values for aissemble-jenkins.


I: Delete file mistakenly committed to dev

carter-cundiff · 2024-09-16T15:48:00Z

foundation/foundation-configuration-store/README.md

@@ -19,30 +19,4 @@ AWS_SECRET_ACCESS_KEY=base-secret-access-key
 ```

 ### Handling Load Status
-The `ConfigLoader` leverages `load-status` to detect whether properties were already fully loaded or only partially loaded due to an interrupted process. The code logic ensures that a `fully-loaded` status is set when properties are loaded successfully. This status is then used to skip loading if the properties were successfully loaded previously, thus avoiding unnecessary reloading during deployment refreshes. For details on this behavior, refer to the implementation code and comments in the `ConfigLoader` class.
-
-### Inject Configuration Value


I: Moved content to antora docs

cwoods-cpointe · 2024-09-16T16:00:45Z

...ain/resources/templates/deployment/configuration-store/v2/configuration-store.values.yaml.vm

@@ -21,7 +21,6 @@ aissemble-configuration-store-chart:
      supplementalVolumeMounts:
        - name: configurations
          mountPath: "/configurations"
-          readOnly: true


q/s: do we not we want to keep the original PVC as read only?

Ideally yes, but it introduces a bug with argoCD (#309). So this is a temp fix for MVP until we get time to further investigate the issue.

ewilkins-csi · 2024-09-16T19:33:52Z

...on/foundation-mda/src/main/java/com/boozallen/aiops/mda/ManualActionNotificationService.java

@@ -944,7 +961,7 @@ private NotificationParams configureNotification(File optionalRoot, String profi
        params.setPomFilePath(deployDir + File.separator + "pom.xml");
        params.setProfileConfiguration("<profile>" + profile + "</profile>");
        params.setPomFile(new File(params.getPomFilePath()));
-        params.setKey(getMessageKey(params.getPomFilePath(), "execution", appName));


A/Q: Feels like it would be more consistent to have the relative path to the POM file here. Can we not relativize against the root like we have elsewhere? (Not sure why it's called "optionalRoot" given the code just uses it directly for setPomFilePath.)

Update to set key with relative path as part of this function. Also update optionalRoot to rootDir.

- Update documentation to live under supporting components - #219 improve manual action message keys

carter-cundiff force-pushed the 322-default-universal-config-store branch from d500e5d to 1a87b9b Compare September 12, 2024 15:51

ewilkins-csi reviewed Sep 12, 2024

View reviewed changes

docs/modules/ROOT/pages/configuration-store.adoc Outdated Show resolved Hide resolved