Skip to content

chore: use certified source for manifest-only test#338

Merged
maxi297 merged 1 commit intomainfrom
maxi297/update-manifest-only-source-from-ci
Feb 14, 2025
Merged

chore: use certified source for manifest-only test#338
maxi297 merged 1 commit intomainfrom
maxi297/update-manifest-only-source-from-ci

Conversation

@maxi297
Copy link
Contributor

@maxi297 maxi297 commented Feb 14, 2025 •
edited by coderabbitai bot
Loading

We expect source-the-guardian-api to be more flaky than source-amplitude as it is a community connector and source-amplitude is actually maintained as a certified connector.

Summary by CodeRabbit

  • Chores
    • Updated our automated testing setup by replacing an older connector with a more current integration.
    • Refined integration status notes to reflect reliability improvements without changing overall functionality.

@maxi297 maxi297 requested a review from aaronsteers February 14, 2025 15:44
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 14, 2025

📝 Walkthrough

Walkthrough

The pull request updates the GitHub Actions workflow by modifying the connector list in the connectors_ci job. Specifically, the connector source-the-guardian-api has been replaced with source-amplitude. The workflow’s underlying structure, job dependencies, conditions, and outputs remain unchanged, and comments regarding the status of other connectors (e.g., flaky or outdated) are preserved. wdyt?

Changes

File Change Summary
.github/workflows/connector-tests.yml Replaced source-the-guardian-api with source-amplitude in the connectors_ci matrix. Comments on connector status remain unchanged.

Possibly related PRs

Suggested labels

enhancement
Would you like to add any additional labels? wdyt?

Suggested reviewers

  • aaronsteers
  • ChristoGrab

Would you like to include these reviewers, or do you have others in mind? wdyt?

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d44aea8 and d97c661.

📒 Files selected for processing (1)
  • .github/workflows/connector-tests.yml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Ruff Format Check
  • GitHub Check: Analyze (python)
🔇 Additional comments (2)
.github/workflows/connector-tests.yml (2)

89-91:

Details

❓ Verification inconclusive

Great choice using a certified connector! But let's verify manifest connector support.

Using source-amplitude (certified) instead of source-the-guardian-api (community) should improve test reliability. However, the TODO comment suggests manifest connectors might have issues with --use-local-cdk. Have you verified that source-amplitude works correctly with the current setup? wdyt?

🏁 Script executed:

#!/bin/bash
# Description: Check if source-amplitude is a manifest-only connector and verify its configuration

# Check if source-amplitude uses the manifest reader
rg -A 5 "ManifestDeclarativeSource" airbyte/airbyte-integrations/connectors/source-amplitude/

# Check for any existing issues or discussions about manifest connector support
gh search issues "source-amplitude manifest --use-local-cdk" --repo airbytehq/airbyte

Length of output: 279

Connector Update Verification

Great choice using a certified connector! The update from source-the-guardian-api to source-amplitude should help improve stability for manifest-only tests. However, since our initial search didn't return evidence about source-amplitude’s manifest configuration (specifically its compatibility with the --use-local-cdk flag), could you please manually verify the following?

  • Lines 89-91: Confirm that source-amplitude is indeed a manifest connector and that it functions correctly with the current --use-local-cdk configuration.
  • Line 161 (if applicable): Ensure that the workflow’s usage of --use-local-cdk is compatible with how manifest connectors operate in your overall test setup.

wdyt?

161-161:

Details

❓ Verification inconclusive

Verify manifest connector support with --use-local-cdk.

The TODO comment suggests that manifest connectors won't work as expected with --use-local-cdk. Should we track this limitation or has it been resolved? wdyt?

🏁 Script executed:

#!/bin/bash
# Description: Search for related issues and recent changes regarding manifest connector support

# Check for related issues
gh search issues "manifest connector --use-local-cdk" --repo airbytehq/airbyte

# Check recent commits that might have added support
git -C airbyte log --grep="manifest.*--use-local-cdk" --since="3 months ago"

Length of output: 220

Manifest Connector Support with --use-local-cdk: Verification Needed
It appears the earlier verification script ran into a directory error (i.e. “fatal: cannot change to 'airbyte'”) and didn’t return any useful information. Could you verify manually (or run an updated search script without the extra directory flag) to confirm whether the limitation with manifest connectors and --use-local-cdk still persists or if it’s been resolved? For instance, running a search like:

rg --ignore-case "manifest connector" -n .
rg --ignore-case "--use-local-cdk" -n .

from the repository root might shed more light. wdyt?

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@maxi297 maxi297 changed the title Use certified source for manifest-only test chore: Use certified source for manifest-only test Feb 14, 2025
aaronsteers
aaronsteers approved these changes Feb 14, 2025
View reviewed changes
Copy link
Contributor

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

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

Sounds good to me! 👍

@aaronsteers aaronsteers changed the title chore: Use certified source for manifest-only test chore: use certified source for manifest-only test Feb 14, 2025
@aaronsteers
Copy link
Contributor

aaronsteers commented Feb 14, 2025

Weird that the semantic PR check isn't rerunning. 😕

@maxi297 maxi297 merged commit dcff834 into main Feb 14, 2025
20 of 22 checks passed
@maxi297 maxi297 deleted the maxi297/update-manifest-only-source-from-ci branch February 14, 2025 22:13
@coderabbitai coderabbitai bot mentioned this pull request Apr 18, 2025
Merged
This was referenced Jul 3, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aaronsteers aaronsteers aaronsteers approved these changes

@coderabbitai coderabbitai[bot] coderabbitai[bot] approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@maxi297 @aaronsteers