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

[Integration]: Serpstack api integration #9381

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

andersooi
Copy link

@andersooi andersooi commented Jun 20, 2024

Description

Created an integration for Serpstack API

Fixes #5641

Type of change

  • ⚡ New feature (non-breaking change which adds functionality)

Verification Process

To ensure the changes are working as expected:

  • Test Location: Please refer to the loom recording attached below
  • Verification Steps: Implemented the integration and documented the process in a README.md file

Additional Media:

  • I have attached a brief loom video or screenshots showcasing the new functionality or change.

Loom Recording

Checklist:

  • My code follows the style guidelines(PEP 8) of MindsDB.
  • I have appropriately commented on my code, especially in complex areas.
  • Necessary documentation updates are either made or tracked in issues.
  • Relevant unit and integration tests are updated or added.

@andersooi andersooi changed the title Serpstack api integration [Integration]: Serpstack api integration Jun 20, 2024
@mindsdbadmin
Copy link
Contributor

mindsdbadmin commented Jun 28, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@andersooi
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@andersooi
Copy link
Author

Hi @ZoranPandovski, there seems to be the following test that is failing but am unsure of what to do on my end to resolve it. If possible, could you give me some guidance on how to go about resolving it?

image

Copy link
Member

@ZoranPandovski ZoranPandovski left a comment

Choose a reason for hiding this comment

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

Thanks @andersooi , don't worry about that failing action. I've added few minor comments

raise ValueError('Query is missing in the SQL query')
if 'type' not in params and hasattr(self, 'default_type'):
params['type'] = self.default_type
api_response = requests.get(self.handler.base_url, params=params)
Copy link
Member

Choose a reason for hiding this comment

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

Can we check for exceptions here?

"""
raise NotImplementedError("Subclasses must implement this method.")

def filter_columns(self, result: pd.DataFrame, query: ast.Select = None):
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comments about what this does?

response = StatusResponse(False)

try:
self.connect()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we don't need to call connect again but simply make a health-check request to serpstack?

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

Successfully merging this pull request may close these issues.

[Integration]: Serpstack API Integration
3 participants