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

Skip building the project for the SQL validation if it isn't needed #831

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

jsnb-devoted
Copy link

@jsnb-devoted jsnb-devoted commented Dec 2, 2024

Change description

Updates the SQL validation check to skip the lookml_models API call if the explores argument is provided to the spectacles sql command.

A non cache hit call to this API takes 20+ minutes to return and in cases where we have made the determination of what explores we want to test -- the API isn't actually necessary.

This change effectively detects whether the explores argument has been provided and if the parameters contain any wildcard selections. If the arguments are single explores (my_model/my_explore) then it skips the API call and builds project with just the provided models/explores.

Type of change

  • Bug fix (fixes an issue)
  • New feature (adds functionality)

Related issues

Checklists

Security

  • Security impact of change has been considered
  • Code follows security best practices and guidelines

Code review

  • Pull request has a descriptive title and context useful to a reviewer

@jsnb-devoted jsnb-devoted marked this pull request as ready for review December 2, 2024 19:06
@jsnb-devoted jsnb-devoted changed the title Devoted sql validation skip build project Skip building the project for the SQL validation if it isn't needed Dec 2, 2024
Copy link
Collaborator

@DylanBaker DylanBaker left a comment

Choose a reason for hiding this comment

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

Hey @jsnb-devoted. Thanks for splitting this out.

While I totally understand how this makes sense for your use case, I think it probably leads to some complications on our end, particularly for how this works in the hosted application.

Any run of Spectacles is typically scoped to a single project (via the --project flag). In a world where we don't build the project and just take explores directly from the filters, you could conceivably be testing explores from multiple different projects (whether they import each other or not). This behaviour isn't necessarily problematic in and of itself, but it would probably need to come with a broader rethinking of how we scope each validator at the project level. I'd be open to exploring this, but I think I'd need @joshtemple's input and we would probably need to work at it a little. Happy to do so, but for transparency we likely won't have time to work on it for at least a few weeks.

@DylanBaker DylanBaker force-pushed the devoted-sql-validation-skip-build-project branch from 881d184 to f5c5b3c Compare December 3, 2024 12:47
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.

2 participants