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

Add support for v1.5 #314

Merged
merged 5 commits into from
May 16, 2023
Merged

Add support for v1.5 #314

merged 5 commits into from
May 16, 2023

Conversation

glsdown
Copy link
Contributor

@glsdown glsdown commented May 12, 2023

Overview

The release of v1.5 broke the upload_invocations macro. This fixes that issue as well as adds in some CI checks that will test the currently supported versions in turn (as well as test running on an empty table, which the double run doesn't test now).

I've additionally split the lint and test actions to try and add some clarity - we will need to add the relevant badge to the README once the lint action is ready.

Update type - breaking / non-breaking

  • Minor bug fix
  • Documentation improvements
  • Quality of Life improvements
  • New features (non-breaking change)
  • New features (breaking change)
  • Other (non-breaking change)
  • Other (breaking change)

What does this solve?

Outstanding questions

Will be interested to see the CI results here as it's not something I can test locally, I'm also not sure how to test it via CI properly either.

It would be good to know if there is a better way of creating the identical tox environments with different deps. It looks like you probably can do it, but I can't see exactly how.

What databases have you tested with?

  • Snowflake
  • Google BigQuery
  • Databricks
  • Spark
  • N/A

@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 16:06 — with GitHub Actions Inactive
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 16:06 — with GitHub Actions Inactive
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 16:06 — with GitHub Actions Inactive
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 16:06 — with GitHub Actions Inactive
@glsdown glsdown added bug Something isn't working priority Priority issue or pull request labels May 12, 2023
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 16:58 — with GitHub Actions Inactive
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 16:58 — with GitHub Actions Inactive
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 16:58 — with GitHub Actions Inactive
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 16:58 — with GitHub Actions Inactive
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 17:00 — with GitHub Actions Inactive
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 17:00 — with GitHub Actions Inactive
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 17:00 — with GitHub Actions Inactive
@glsdown glsdown temporarily deployed to Approve Integration Tests May 12, 2023 17:00 — with GitHub Actions Inactive
Copy link
Member

@dluftspring dluftspring left a comment

Choose a reason for hiding this comment

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

👍 no issues here.

Documenting for future PRs because they aren't logically part of this fix

  1. The shell script chaining that happens in our workflow steps between getting the file changes and calling sqlfluff lint probably has to go. We've copied it all over the place from our project template and it's not very readable. I'm 90% sure it's been done better in a marketplace action that will save us lines of code
  2. More of a style thing. Do we want to combine the workflows into single yaml files based on which events trigger them. This is sort of how GitHub wants you to do it because then you can recycle the name of the status badges e.g."PR Workflows" would have job names "lint" and "test".

Point (1) I feel strongly about point (2) is a take it or leave it type thing but something we should at least look at. We choose between proliferation of yaml files or having long yaml files.

@glsdown glsdown merged commit 95e958a into main May 16, 2023
@glsdown glsdown deleted the feat/add-support-for-1.5 branch May 16, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority Priority issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants