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

fix: remove unused CONTAINER_ORCHESTRATOR_ENABLED var #20261

Merged
merged 2 commits into from
Dec 8, 2022
Merged

fix: remove unused CONTAINER_ORCHESTRATOR_ENABLED var #20261

merged 2 commits into from
Dec 8, 2022

Conversation

c-p-b
Copy link
Contributor

@c-p-b c-p-b commented Dec 8, 2022 •
edited
Loading

What

The behavior that this environment variable affects was removed in this pr. Therefore, this environment variable is no longer used and can be removed entirely.

How

Remove all references to the environment variable from code and charts

@c-p-b c-p-b requested a review from benmoriceau December 8, 2022 19:14
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/worker Related to worker kubernetes labels Dec 8, 2022
@c-p-b c-p-b temporarily deployed to more-secrets December 8, 2022 19:15 — with GitHub Actions Inactive
@c-p-b c-p-b temporarily deployed to more-secrets December 8, 2022 19:15 — with GitHub Actions Inactive
benmoriceau
benmoriceau reviewed Dec 8, 2022
View reviewed changes
airbyte-config/config-models/src/main/java/io/airbyte/config/EnvConfigs.java
@@ -1043,11 +1042,6 @@ public Set<Integer> getTemporalWorkerPorts() {
return Arrays.stream(ports.split(",")).map(Integer::valueOf).collect(Collectors.toSet());
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is is missing an update to the Configs.java file as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@c-p-b c-p-b temporarily deployed to more-secrets December 8, 2022 21:19 — with GitHub Actions Inactive
@c-p-b c-p-b temporarily deployed to more-secrets December 8, 2022 21:20 — with GitHub Actions Inactive
@c-p-b c-p-b merged commit ce29361 into master Dec 8, 2022
@c-p-b c-p-b deleted the remove-co-env-var branch December 8, 2022 22:27
@github-actions github-actions bot mentioned this pull request Dec 8, 2022
Closed
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Dec 9, 2022
Merged
@ghost
Copy link

ghost commented Dec 9, 2022 •
edited by ghost
Loading

@cpdeethree Faced an error due to this PR where installation failed with the following error:

Error: INSTALLATION FAILED: template: airbyte/templates/env-configmap.yaml:67:42: executing "airbyte/templates/env-configmap.yaml" at <.Values.worker.containerOrchestrator.image>: nil pointer evaluating interface {}.image

Fixed it by adding back the following code in values.yaml file
worker.containerOrchestrator.image: ""

edit: Reference issue #20282

c-p-b pushed a commit that referenced this pull request Dec 9, 2022
restore containerOrchestrator image value
f2fa0c7 
This was mistakenly removed as part of #20261
@c-p-b c-p-b mentioned this pull request Dec 9, 2022
Merged
@git-phu
Copy link
Contributor

git-phu commented Dec 12, 2022

this isn't right
container orchestrator is required for cloud and is used here:

airbyte/airbyte-workers/src/main/java/io/airbyte/workers/config/ContainerOrchestratorConfigBeanFactory.java

Lines 46 to 47 in f76833e

@Requires(property = "airbyte.container.orchestrator.enabled",
value = "true")

c-p-b added a commit that referenced this pull request Dec 13, 2022
@c-p-b
Revert "fix: remove unused CONTAINER_ORCHESTRATOR_ENABLED var (#20261)"
98c0cf3 
This reverts commit ce29361.
c-p-b pushed a commit that referenced this pull request Dec 13, 2022
restore containerOrchestrator image value (#20313)
e7c3fbf 
* Revert "fix: remove unused CONTAINER_ORCHESTRATOR_ENABLED var (#20261)"

This reverts commit ce29361.

* docs: add additional commentary on flag usage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benmoriceau benmoriceau benmoriceau approved these changes

Assignees
No one assigned
Labels
area/platform issues related to the platform area/worker Related to worker kubernetes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@c-p-b @git-phu @benmoriceau @octavia-squidington-iv