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

chore(airbyte-ci): move common_utils into ci_credentials #52643

Merged
merged 17 commits into from
Jan 31, 2025

Conversation

natikgadzhi
Copy link
Contributor

@natikgadzhi natikgadzhi commented Jan 30, 2025

What

This PR removes common_utils, a package that was only used in ci_credentials explicitly. I've moved the logger and google_api client into ci_credentials. Then, to clean things up:

  • I've re-locked all packages that used connector_ops because they had transitive dependency on common_utils.
  • Cleaned up READMEs
  • Made new versions of all affected packages
  • Removed common_utils from the list of CI checks

This also removes required reviewers checks that connector_ops provided. I strongly believe we should use GitHub-native actions that other folks maintain to setup required reviewers based on CODEOWNERS instead IF we actually need them.

@natikgadzhi natikgadzhi requested a review from a team as a code owner January 30, 2025 00:32
Copy link

vercel bot commented Jan 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 31, 2025 1:23am

@natikgadzhi
Copy link
Contributor Author

This is mostly housekeeping, I am trying to reduce our footprint as much as possible.

@natikgadzhi
Copy link
Contributor Author

@ian-at-airbyte heads up, in your PR you changed qa-checks docs that are autogenerated.

I will rewrite it here, but I'm happy to help you port your changes to the template and regenerate them.

@natikgadzhi
Copy link
Contributor Author

I'm getting a couple of type check errors which I am willing to ignore, and ci_credentials tests fine locally, but failed on CI when I last ran it. Weird.

@aaronsteers
Copy link
Collaborator

@natikgadzhi - This plagued me before - thank you Natik!!

@aaronsteers
Copy link
Collaborator

I strongly believe we should use GitHub-native actions that other folks maintain to setup required reviewers based on CODEOWNERS instead IF we actually need them.

💯

@ian-at-airbyte
Copy link
Contributor

@ian-at-airbyte heads up, in your PR you changed qa-checks docs that are autogenerated.

I will rewrite it here, but I'm happy to help you port your changes to the template and regenerate them.

Thanks Natik. I did not realize this file was autogenerated.

@natikgadzhi
Copy link
Contributor Author

The tests on ci_credentials are testing in a funny way — I think tests on CI can't intercept stderr for some reason. I will merge this because they work locally just fine.

@natikgadzhi
Copy link
Contributor Author

Oh wait I can't quite merge this, I think it tests all packages, not just the changed ones, so it will start yelling red on everything

@natikgadzhi natikgadzhi disabled auto-merge January 31, 2025 01:17
@natikgadzhi
Copy link
Contributor Author

Upd: nope, master CI actually has the same failures (outside of logger failures) and I'm okay with them. We might have to fix this down the line, but ci_credentials being internal tool, this is ok.

@natikgadzhi natikgadzhi merged commit b70b57b into master Jan 31, 2025
18 of 21 checks passed
@natikgadzhi natikgadzhi deleted the ng/delete-common-utils branch January 31, 2025 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants