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

conectors-qa: verify connector breaking changes are at least 7 days ahead #35387

Merged
merged 3 commits into from
Apr 8, 2024
Merged

conectors-qa: verify connector breaking changes are at least 7 days ahead #35387

merged 3 commits into from
Apr 8, 2024

Conversation

natikgadzhi
Copy link
Contributor

@natikgadzhi natikgadzhi commented Feb 17, 2024 •
edited
Loading

Summary

This PR adds a check to connectors-qa by @alafanechere to verify that if a connector has a breaking change defined in it's most recent version, than it's upgradeDeadline is at least 7 days away.

Closes airbytehq/airbyte-internal-issues#6294

That's assuming that we actually run connectors-qa on all connectors, including certified 🙃

Impact

  • It's a CI check, no product feature changes, safe to ship.
  • I verified that it works on a few examples locally.

TODOs

  • A unit test would be nice.

@natikgadzhi natikgadzhi requested a review from a team as a code owner February 17, 2024 06:31
@vercel Vercel
Copy link

vercel bot commented Feb 17, 2024 •
edited
Loading

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 Apr 8, 2024 3:25pm

@natikgadzhi natikgadzhi self-assigned this Feb 17, 2024
@alafanechere alafanechere force-pushed the augustin/02-10-qa_checks_v2 branch 3 times, most recently from e418127 to 8f55503 Compare February 19, 2024 09:26
Base automatically changed from augustin/02-10-qa_checks_v2 to master February 19, 2024 11:43
alafanechere
alafanechere requested changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@alafanechere alafanechere left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think you have to rebase your branch on master now that I merged in the package. Please bump the package version and add a changelog entry in the README.

I like this check addition but I'm concerned that this check will fail if we periodically run it on master.
This package is able to create a global JSON reports of all our connectors QA check result. The idea was to periodically generate it to spot which connector is not passing the QA checks.

I think we should make this check fetch the OSS registry, if the current connector version is in it we can skip this check as it'd mean the connector version is already released.

airbyte-ci/connectors/connectors_qa/connectors_qa/checks/metadata.py Outdated Show resolved Hide resolved
airbyte-ci/connectors/connectors_qa/connectors_qa/checks/metadata.py Outdated Show resolved Hide resolved
airbyte-ci/connectors/connectors_qa/connectors_qa/checks/metadata.py Outdated Show resolved Hide resolved
airbyte-ci/connectors/connectors_qa/connectors_qa/checks/metadata.py Outdated Show resolved Hide resolved
@natikgadzhi
Copy link
Contributor Author

@alafanechere yep, the main goal of this is to run on pull requests, not on main. I would prefer to not overcomplicate and configure running only on pull requests explicitly, at least for this check. Is that an option?

Also, holy crap, I learned a few things — thank you for the review! 👏🏼

@natikgadzhi natikgadzhi force-pushed the ng/connector-qa-breaking-changes-deadline branch from d253539 to c679df0 Compare March 7, 2024 05:04
@natikgadzhi
Copy link
Contributor Author

@alafanechere alright, I've cleaned up the things you pointed out (thank you!) but have not added tests yet.

Help me out — how do we make this check optional on nightly runs? @katmarkham really wanted that one ;)

@natikgadzhi
Copy link
Contributor Author

@alafanechere boop. I think aside from borked formatting I should be close! Want to pair tomorrow on both of my PRs?

@alafanechere alafanechere force-pushed the ng/connector-qa-breaking-changes-deadline branch from c679df0 to 533316d Compare April 4, 2024 08:17
@alafanechere alafanechere changed the base branch from master to augustin/04-04-connectors-qa_Introduce_the_Check.run_on_released_connectors_flag April 4, 2024 08:17
@alafanechere Graphite App
Copy link
Contributor

alafanechere commented Apr 4, 2024 •
edited by natikgadzhi
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @natikgadzhi and the rest of your teammates on Graphite Graphite

@alafanechere alafanechere mentioned this pull request Apr 4, 2024
Merged
natikgadzhi
natikgadzhi commented Apr 4, 2024
View reviewed changes
Copy link
Contributor Author

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

Seems there's an angry unit test and I'm not sure why, but otherwise I'm happy with the way you added the check guard, @alafanechere.

I can take a look in a bit and get this over the finish line if you wish.

airbyte-ci/connectors/connectors_qa/pyproject.toml
@@ -1,6 +1,6 @@
[tool.poetry]
name = "connectors-qa"
version = "1.1.0"
version = "1.2.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you <3

docs/contributing-to-airbyte/resources/qa-checks.md
@@ -43,6 +43,11 @@ Connectors must have a language tag in their metadata. It must be set in the `ta
*Applies to the following connector languages: python, low-code*

Python connectors must have a CDK tag in their metadata. It must be set in the `tags` field in metadata.yaml. The values can be `cdk:low-code`, `cdk:python`, or `cdk:file`.
### Breaking change deadline should be a week in the future
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for running the generator!

@octavia-squidington-iv octavia-squidington-iv requested a review from a team April 4, 2024 20:54
@alafanechere alafanechere force-pushed the augustin/04-04-connectors-qa_Introduce_the_Check.run_on_released_connectors_flag branch from 8deb9a3 to 4da8624 Compare April 8, 2024 15:01
@alafanechere alafanechere force-pushed the ng/connector-qa-breaking-changes-deadline branch from 533316d to acfcd3c Compare April 8, 2024 15:01
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Apr 8, 2024
Base automatically changed from augustin/04-04-connectors-qa_Introduce_the_Check.run_on_released_connectors_flag to master April 8, 2024 15:12
@alafanechere alafanechere force-pushed the ng/connector-qa-breaking-changes-deadline branch from acfcd3c to ad50e1e Compare April 8, 2024 15:20
@alafanechere alafanechere enabled auto-merge (squash) April 8, 2024 15:21
@alafanechere alafanechere merged commit b753ade into master Apr 8, 2024
29 checks passed
@alafanechere alafanechere deleted the ng/connector-qa-breaking-changes-deadline branch April 8, 2024 15:29
@lazebnyi lazebnyi mentioned this pull request Apr 22, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alafanechere alafanechere alafanechere approved these changes

Assignees

@natikgadzhi natikgadzhi

Labels
area/documentation Improvements or additions to documentation team/extensibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@natikgadzhi @alafanechere @octavia-squidington-iii