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.

Sign up for GitHub

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

Jump to bottom

[charts/airbyte-cron] Cleanup env vars #18787

Merged
merged 3 commits into from
Nov 1, 2022
Merged

[charts/airbyte-cron] Cleanup env vars #18787

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6e20b4
[charts/airbyte-cron] Cleanup env vars
perangel Nov 1, 2022
e6ac712
Merge branch 'master' into perangel/fix/cleanup-airbyte-cron-helm-dep…
perangel Nov 1, 2022
18d9682
Remove unused env var
perangel Nov 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions charts/airbyte-cron/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,6 @@ spec:
imagePullPolicy: "{{ .Values.image.pullPolicy }}"
env:
{{- if eq .Values.global.deploymentMode "oss" }}
- name: AIRBYTE_ROLE
valueFrom:
configMapKeyRef:
name: {{ .Release.Name }}-airbyte-env
key: AIRBYTE_ROLE
- name: AIRBYTE_VERSION
valueFrom:
configMapKeyRef:
Expand Down Expand Up @@ -85,21 +80,11 @@ spec:
configMapKeyRef:
name: {{ .Release.Name }}-airbyte-env
key: CRON_MICRONAUT_ENVIRONMENTS
- name: REMOTE_CONNECTOR_CATALOG_URL
valueFrom:
configMapKeyRef:
name: {{ .Release.Name }}-airbyte-env
key: REMOTE_CONNECTOR_CATALOG_URL
- name: TRACKING_STRATEGY
valueFrom:
configMapKeyRef:
name: {{ .Release.Name }}-airbyte-env
key: TRACKING_STRATEGY
- name: UPDATE_DEFINITIONS_CRON_ENABLED
valueFrom:
configMapKeyRef:
name: {{ .Release.Name }}-airbyte-env
key: UPDATE_DEFINITIONS_CRON_ENABLED
Comment on lines -88 to -102
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about airbyte_role, but for these two (remote_connector_catalog_url and update_definitions_cron_enabled), while they could be set on OSS, they're mainly meant for cloud usage so it's probably ok to remove for now if it's causing an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@perangel is there a reason not to add them to the configmap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that AIRBYTE_ROLE is declared in https://github.com/airbytehq/airbyte/blob/master/airbyte-cron/src/main/resources/application.yml#L69. Anything not declared by the ConfigMap can still be provided via env_vars or global.env_vars so there's still a means to pass them through. But since these keys are definitely not in the ConfigMap, it should be safe to remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@perangel is there a reason not to add them to the configmap?

None that I am aware of, but I would like to get a second opinion since I see cases where other config keys that are declared in microaut apps are also not present in the ConfigMap

Copy link
Contributor

@xpuska513 xpuska513 Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As for me, I've never seen those variables present in OSS deployments, only in cloud one, so if devs are ok with dropping those(meaning that it'll be safe), then we can drop them and test the deployment out.
For this you need to run the following:

mv charts/airbyte/Chart.yaml charts/airbyte/Chart.yaml.old
mv charts/airbyte/Chart.yaml.test charts/airbyte/Chart.yaml 
cd charts/airbyte && helm dep update && cd -
helm upgrade --install --debug airbyte charts/airbyte

- name: WORKFLOW_FAILURE_RESTART_DELAY_SECONDS
valueFrom:
configMapKeyRef:
Expand Down