-
Notifications
You must be signed in to change notification settings - Fork 9
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
#484 Feature: integrate Universal Config store in the hive metastore service #494
Conversation
...helm/extensions-helm-spark-infrastructure/aissemble-hive-metastore-service-chart/values.yaml
Outdated
Show resolved
Hide resolved
...dation-mda/src/main/resources/templates/deployment/hive-metastore-service/hive.properties.vm
Outdated
Show resolved
Hide resolved
foundation/foundation-mda/src/test/resources/specifications/data-records-generation.feature
Outdated
Show resolved
Hide resolved
...java/com/boozallen/aissemble/upgrade/migration/v1_11_0/HiveUniversalConfigYAMLMigration.java
Outdated
Show resolved
Hide resolved
...java/com/boozallen/aissemble/upgrade/migration/v1_11_0/HiveUniversalConfigYAMLMigration.java
Outdated
Show resolved
Hide resolved
...java/com/boozallen/aissemble/upgrade/migration/v1_11_0/HiveUniversalConfigYAMLMigration.java
Outdated
Show resolved
Hide resolved
...java/com/boozallen/aissemble/upgrade/migration/v1_11_0/HiveUniversalConfigYAMLMigration.java
Outdated
Show resolved
Hide resolved
...java/com/boozallen/aissemble/upgrade/migration/v1_11_0/HiveUniversalConfigYAMLMigration.java
Outdated
Show resolved
Hide resolved
1aaa91c
to
bfbc5d4
Compare
...ation/foundation-upgrade/src/test/resources/specifications/v1_11_0/hive-uc-migration.feature
Outdated
Show resolved
Hide resolved
...ation/foundation-upgrade/src/test/resources/specifications/v1_11_0/hive-uc-migration.feature
Outdated
Show resolved
Hide resolved
bfbc5d4
to
e9503aa
Compare
b3c234c
to
44c9e26
Compare
...ns-helm-spark-infrastructure/aissemble-hive-metastore-service-chart/templates/configmap.yaml
Outdated
Show resolved
Hide resolved
...java/com/boozallen/aissemble/upgrade/migration/v1_11_0/HiveUniversalConfigYAMLMigration.java
Outdated
Show resolved
Hide resolved
@@ -15,11 +15,16 @@ image: | |||
dockerRepo: "ghcr.io/" | |||
|
|||
mysql: | |||
commonLabels: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A: Seems like we might want to be more precise and attach labels only to the resources that need it instead of using this commonLables
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! I wish I would inject label for specific to kind, but looking at mysql's logic all metadata's label logic is embedded to use only commonLabels. see here for example code snippet for mysql chart
{{- if eq .Values.architecture "replication" }}
apiVersion: {{ include "common.capabilities.statefulset.apiVersion" . }}
kind: StatefulSet
metadata:
name: {{ include "mysql.secondary.fullname" . }}
namespace: {{ include "common.names.namespace" . | quote }}
labels: {{- include "common.labels.standard" ( dict "customLabels" .Values.commonLabels "context" $ ) | nindent 4 }}
app.kubernetes.io/component: secondary
{{- if .Values.commonAnnotations }}
annotations: {{- include "common.tplvalues.render" ( dict "value" .Values.commonAnnotations "context" $ ) | nindent 4 }}
{{- end }}
spec:
...java/com/boozallen/aissemble/upgrade/migration/v1_11_0/HiveUniversalConfigYAMLMigration.java
Outdated
Show resolved
Hide resolved
data: | ||
# Add all the default properties from the local values.yaml to the ConfigMap | ||
# Then check if there are any downstream properties and add them as well | ||
metastore-site.xml: | | ||
{{- $basePropDict := dict }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: Just use one dictionary. Write the base keys first, then write the props. That will automatically cause anything duplicate values in properties
to take precedence over baseProperties
without complex logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'm not really sure why we need these dictionaries at all. Can you not just do:
{{- if not hasProperty .Values.configMap.metastoreServiceConfig.properties $baseProperty.name }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm bit new to helm syntax but from what I looked at the doc it has some limitation in creating variable and function so I had to go with dicts but open to idea and see if there's easy way to do it.
is hasProperty custom method? wasn't sure it was doable, even if it was custom methods,, shouldn't hasProperty need dict to accomplish the logic?
Im trying to accomplish 3 use case,
- add baseProperty that doesn't exist in properties
- add properties that doesn't exist in baseProperties
- Add properties that both exist in baseProperties and properties ( take properties as precedence )
it's bit hard to accomplish contain logic in helm because property schema is list of dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can def cut follow up ticket if this requires refactoring given that this ticket is past DPC1!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added https://jira.boozallencsn.com/browse/AIOPS-3838 as a follow up and will be working this
...dation-mda/src/main/resources/templates/deployment/hive-metastore-service/hive.properties.vm
Outdated
Show resolved
Hide resolved
foundation/foundation-mda/src/test/java/com/boozallen/aiops/mda/generator/PropertiesStep.java
Show resolved
Hide resolved
f9e332f
to
6ffe878
Compare
...java/com/boozallen/aissemble/upgrade/migration/v1_11_0/HiveUniversalConfigYAMLMigration.java
Outdated
Show resolved
Hide resolved
8e8ccd7
to
859c8f5
Compare
...len/aissemble/upgrade/migration/v1_11_0/SparkInfrastructureUniversalConfigYAMLMigration.java
Show resolved
Hide resolved
...len/aissemble/upgrade/migration/v1_11_0/SparkInfrastructureUniversalConfigYAMLMigration.java
Show resolved
Hide resolved
b27ac10
to
ddbc3ad
Compare
#484