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.

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

add a chance for user to specify an additional private image registry… #16969

Closed
wants to merge 21 commits into from

Conversation

cloorc
Copy link

@cloorc cloorc commented Sep 21, 2022

Add a chance for user to specify an additional private image registry without change seed_sources/seed_destinations

What

  • Add no-op gradle task distTar to prevent from failre of missing task in vscode(related modules: airbyte-cli/airbyte-config/airbyte-db/airbyte-temporal/airbyte-webapp).
  • Ignore aoto-generated classes from yaml(**/bin/)
  • Add an additional options to specify private image registry beside of imagePullSecrets

How

Add registry prefix in ProcessFactory on create new container if the environment JOB_KUBE_MAIN_CONTAINER_IMAGE_REGISTRY has been assigned with a valid registry string.

Recommended reading order

  1. airbyte\airbyte-workers\src\main\java\io\airbyte\workers\WorkerConfigs.java
  2. airbyte\airbyte-workers\src\main\java\io\airbyte\workers\process\DockerProcessFactory.java
  3. airbyte\airbyte-workers\src\main\java\io\airbyte\workers\process\KubeProcessFactory.java
  4. airbyte\airbyte-workers\src\main\java\io\airbyte\workers\config\WorkerConfigurationBeanFactory.java

User Impact

This option is transparent and optional for airbyte users.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

… without change seed_sources/seed_destinations
@cloorc cloorc requested a review from a team as a code owner September 21, 2022 12:50
@github-actions github-actions bot added area/platform issues related to the platform area/worker Related to worker area/frontend Related to the Airbyte webapp labels Sep 21, 2022
@teallarson teallarson removed the request for review from a team September 22, 2022 14:10
@teallarson
Copy link
Contributor

Hi there! I've removed the request for frontend review and added a tag so this gets seen by our Platform Workflow team. They'll be better able to review this.

@marcosmarxm
Copy link
Member

marcosmarxm commented Sep 26, 2022

@cloorc sorry about delay in your review. You're doing 3 different things your PR correct? Can you break into 3 different PR so will be easier to ask team to review them?

Also I strongly recommend to you open first an Issue and describe with all information what it the current problem you'll try to fix/add feature.

@cloorc
Copy link
Author

cloorc commented Sep 27, 2022

@cloorc sorry about delay in your review. You're doing 3 different things your PR correct? Can you break into 3 different PR so will be easier to ask team to review them?

Also I strongly recommend to you open first an Issue and describe with all information what it the current problem you'll try to fix/add feature.

Sorry to reply late. Yes, there are three changes about this pull request:

  1. Ignore all auto-generated sources under bin/ from schema;
  2. Specify an no-op gradle build task in several projects to prevent from failure of gradle project scanning in vscode;
  3. Support private image registry(this is the main idea in this pull request);

Let me remove first two changes and continue. Thanks!

@github-actions github-actions bot removed the area/frontend Related to the Airbyte webapp label Sep 27, 2022
@cloorc
Copy link
Author

cloorc commented Sep 27, 2022

@cloorc sorry about delay in your review. You're doing 3 different things your PR correct? Can you break into 3 different PR so will be easier to ask team to review them?

Also I strongly recommend to you open first an Issue and describe with all information what it the current problem you'll try to fix/add feature.

Hi, my latest commit has removed two not-strictly-related modifications. Please have a look! 🥇

@marcosmarxm
Copy link
Member

Thanks @cloorc sorry the delay! I'll ask someone to review it soon

@davinchia
Copy link
Contributor

@cloorc curious, what are you trying to solve with being able to specify a private image registry? Is it the need to host airbyte provided connector images in a private docker registry?

i.e. I want an easy way to configure my Airbyte instance to pull from a private registry without having to configure these images as custom connectors. (Since we can do so today via the custom connector route except it's less ergonomic)

@cloorc
Copy link
Author

cloorc commented Oct 10, 2022

@cloorc curious, what are you trying to solve with being able to specify a private image registry? Is it the need to host airbyte provided connector images in a private docker registry?

i.e. I want an easy way to configure my Airbyte instance to pull from a private registry without having to configure these images as custom connectors. (Since we can do so today via the custom connector route except it's less ergonomic)

The original requirement is deploying airbyte on our private Kubernetes cluster. Our network connection to docker hub is not ideal so we have to transfer theses images to our private registry one by one manually(together with something like VPN infrastructure temporarily).

We tried to accellerate the access to docker hub by adding additional accelerating registry but it is ugly to add registry on each node in the target kubernetes cluster.

This commit just allow us to specify a registry prefix(a private registry) to pull required connector images without any change on container infrastructure(both docker & k8s), especially change nothing to airbyte instance(as its sources/desitinations are
enough for most cases already).

@davinchia
Copy link
Contributor

@cloorc understood. Thanks for the explanation!

Are the private registries only for the connector images? Or are they for all images Airbyte uses? I think this PR doesn't cover the rest of the images we use as part of Kubernetes. Is that also important?

@cloorc
Copy link
Author

cloorc commented Oct 14, 2022

@cloorc understood. Thanks for the explanation!

Are the private registries only for the connector images? Or are they for all images Airbyte uses? I think this PR doesn't cover the rest of the images we use as part of Kubernetes. Is that also important?

Yes. Airbytes' executables related images could be specified manually in chart already. So this PR should only works with source/desitination images.

@marcosmarxm
Copy link
Member

Hello 👋, first thank you for this amazing contribution.

We really appreciate the effort you've made to improve the project.
We ask you patience for the code review. Last month our team was focused on Hacktoberfest event and that probably left some PR without the proper feedback. And this week, due to the Thanksgiving US Holiday, most our team is out of office with their families. Another important piece of information why code won't be merge this week is: as a safety measure the core team has decided to freeze merging code to main branch to keep the release stable. Next week we'll return to you with the proper code review and update the status of your contribution.

If you have any questions feel free to send me a message in Slack!
Thanks!

@marcosmarxm
Copy link
Member

Hello 👋:skin-tone-2: and thank you for your contribution!

Airbyte has instituted a code freeze between 19 and 30 December, to make sure there are no disruptions during the holidays.
Because of this, reviewing and merging your contribution may take longer than usual.
We apologize for the delay, but we want everyone to have a quiet and happy holiday.

If you have any questions or need further clarification, please don't hesitate to ping via Slack.

@cloorc
Copy link
Author

cloorc commented Dec 21, 2022

Hello 👋:skin-tone-2: and thank you for your contribution!

Airbyte has instituted a code freeze between 19 and 30 December, to make sure there are no disruptions during the holidays. Because of this, reviewing and merging your contribution may take longer than usual. We apologize for the delay, but we want everyone to have a quiet and happy holiday.

If you have any questions or need further clarification, please don't hesitate to ping via Slack.

Hi @marcosmarxm, I've rechecked my commits and solved the latest conflicts with airbytemq:master, please have a look.

@AKuzyashin
Copy link

Any updates here ?

@cloorc
Copy link
Author

cloorc commented Jan 11, 2023

Any updates here ?

Not yet. No body has spare time to review this PR.

Copy link
Member

@colesnodgrass colesnodgrass left a comment

Choose a reason for hiding this comment

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

This PR contains several unresolved git conflicts (all of them tagged with the comment unresolved git conflict) that need to be resolved before this PR could be considered for acceptance.

@octavia-squidington-iii octavia-squidington-iii removed area/worker Related to worker area/platform issues related to the platform labels Mar 10, 2023
@cgardens
Copy link
Contributor

Hi @cloorc ! Thanks for your contribution. We have moved our platform to a new public repo. Would you be willing to move your PR over there, please? If you do that and tag me, I'm happy to make sure we get this upstreamed. Sorry for the delay on review, but the code move was disruptive to our review process. Going to close this PR for now since the code has moved.

@cnogradi
Copy link

cnogradi commented Oct 23, 2023

Hi @cloorc ! Thanks for your contribution. We have moved our platform to a new public repo. Would you be willing to move your PR over there, please? If you do that and tag me, I'm happy to make sure we get this upstreamed. Sorry for the delay on review, but the code move was disruptive to our review process. Going to close this PR for now since the code has moved.

@cgardens Per your request I adapted this change and move it to the new platform repo: airbytehq/airbyte-platform#284. Can you review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants