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

Extension compliance updates #35

Merged
merged 2 commits into from
Apr 12, 2023
Merged

Extension compliance updates #35

merged 2 commits into from
Apr 12, 2023

Conversation

javaducky
Copy link
Contributor

This PR does multiple things:

  • minor refactoring of examples
  • upgrades to the latest version of k6 and other dependencies
  • enables linting in CI workflow
  • addresses any linter issues
  • adds unit testing using SQLLite
  • tweaks docker-run.sh helper script to allow pass-through of additional k6 arguments, e.g. --quiet

Fixes #34 and partially addresses issues defined in #5.

We are standardizing the directory containing example usage and integration tests.

Signed-off-by: Paul Balogh <javaducky@gmail.com>
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Thanks for this @javaducky! 🙇 It's great that we have at least one proper test now 😅🎉

I left some minor and not-so-minor suggestions. Mainly I think we should be careful with the SQL.Query() changes.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
# Multi-stage build to generate custom k6 with extension
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should drop this Dockerfile now that xk6 has one. And update the note in the README with the right command to build xk6-sql with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xk6 image does not yet support CGO_ENABLED as required for SQLite3 support. I created an issue in xk6 for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned in the issue, this doesn't require any changes in the grafana/xk6 image.

So I'd still prefer removing this Dockerfile and the docker-run.sh script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should keep this until xk6 docker image is actually able to build the binary. Once we can build using CGO_ENABLED, I'll be more than happy to update the documentation and remove the Dockerfile and docker-run.sh files.

@@ -2,13 +2,13 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this script, I would remove it. The way to simplify the Docker CLI is to use Docker Compose, but for this simple extension, I don't think we need it either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving this as the CGO support is required to switch to the xk6 docker image.

sql.go Outdated Show resolved Hide resolved
sql.go Outdated Show resolved Hide resolved
sql_test.go Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
sql_test.go Outdated Show resolved Hide resolved
@imiric imiric mentioned this pull request Apr 11, 2023
Fixes #34 and relates to #5

Signed-off-by: Paul Balogh <javaducky@gmail.com>
@javaducky javaducky merged commit 5910271 into master Apr 12, 2023
9 checks passed
@javaducky javaducky deleted the paul/compliance-updates branch April 12, 2023 18:25
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.

Update for Extensions Compliance
2 participants