-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Improve JOB_POD variable naming + improve doc about memory management #9048
Conversation
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.
Thanks for taking this on. Agree leaving pod in the name was confusing.
Instead of only removing pod, what about introducing a convention where Kube only Job variables have the JOB_KUBE_
prefix? e.g. JOB_KUBE_NAMESPACE
, JOB_KUBE_SOCAT_IMAGE
etc. It would help differentiate between general job variables, and job variables that only apply to K8s.
Agreed 🎯 Done |
airbyte-config/models/src/main/java/io/airbyte/config/EnvConfigs.java
Outdated
Show resolved
Hide resolved
airbyte-config/models/src/main/java/io/airbyte/config/EnvConfigs.java
Outdated
Show resolved
Hide resolved
airbyte-config/models/src/main/java/io/airbyte/config/EnvConfigs.java
Outdated
Show resolved
Hide resolved
airbyte-config/models/src/main/java/io/airbyte/config/EnvConfigs.java
Outdated
Show resolved
Hide resolved
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.
Some more renames!
Please also make sure the methods match the variable names.
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.
Thanks!
What
POD
in their name but are not specific to Kubernetes deployment: I suggest removingPOD
from their name.How
JOB_POD_*
toJOB_*
env var when they are not strictly related to K8S deploymentsJOB_MAIN_CONTAINER_MEMORY_REQUEST
andJOB_MAIN_CONTAINER_MEMORY_LIMIT
variable.Recommended reading order
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changes