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

feat(contrib/qxip): add contrib/qxip/clickhouse package #5351

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

lmangani
Copy link
Contributor

This PR adds contrib/qxip/clickhouse package to interact with ClickHouse HTTP APIs. The contrib adds no code and only wraps flux functions. Included readme explains the functionality.

Done checklist

  • No additional Code.

@lmangani lmangani requested review from a team as code owners December 20, 2022 01:27
@lmangani lmangani requested review from wolffcm and lwandzura and removed request for a team December 20, 2022 01:27
Copy link

@wolffcm wolffcm left a comment

Choose a reason for hiding this comment

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

You'll need to make the change I mention below for this to pass CI. I think you may also need to run make generate?

stdlib/packages.go Outdated Show resolved Hide resolved
@wolffcm
Copy link

wolffcm commented Dec 20, 2022

I should add: the change looks good otherwise and thank you for taking the time to do it.

@lmangani
Copy link
Contributor Author

Thanks for the guidance @wolffcm, I appreciate it! I have one more plugin in our contrib folder (flux only, no go changes) would you suggest I submit both in this PR or open separate ones?

@lmangani lmangani requested review from wolffcm and removed request for lwandzura December 20, 2022 11:39
@wolffcm
Copy link

wolffcm commented Dec 20, 2022

@lmangani I'm still seeing a failure in CI. I'm not sure if you can see them since you're not in our org, but here it is:

generated code is not accurate, please run make generate
ragel version is: Ragel State Machine Compiler version 7.0.1 November 2020
Copyright (c) 2001-2019 by Adrian Thurston et al.
Files changed:
 M libflux/go/libflux/buildinfo.gen.go
diff --git a/libflux/go/libflux/buildinfo.gen.go b/libflux/go/libflux/buildinfo.gen.go
index 8bbf4220..599a4e8a 100644
Binary files a/libflux/go/libflux/buildinfo.gen.go and b/libflux/go/libflux/buildinfo.gen.go differ
make: *** [Makefile:99: checkgenerate] Error 1

It looks like you still need to run make generate. If you already have, let me know and I can help get you sorted.

You should be able to run these lines from the CI config to ensure that your PR will pass CI:

- run: make checkgenerate
- run: make checkfmt
- run: make checktidy
- run: make checkdocs
- run: make checkrelease
- run: make vet
- run: GOGC=50 make staticcheck
- run: make libflux-wasm
- run: make test GO_TEST_FLAGS='-coverprofile=coverage.txt -covermode=atomic' GO_TAGS=assert

@wolffcm
Copy link

wolffcm commented Dec 20, 2022

I have one more plugin in our contrib folder (flux only, no go changes) would you suggest I submit both in this PR or open separate ones?

I don't have a preference, feel free to go with whatever is easiest for you.

stdlib/contrib/qxip/clickhouse/clickhouse.flux Outdated Show resolved Hide resolved
stdlib/contrib/qxip/clickhouse/clickhouse.flux Outdated Show resolved Hide resolved
stdlib/contrib/qxip/clickhouse/clickhouse.flux Outdated Show resolved Hide resolved
stdlib/contrib/qxip/clickhouse/clickhouse.flux Outdated Show resolved Hide resolved
stdlib/contrib/qxip/clickhouse/clickhouse.flux Outdated Show resolved Hide resolved
stdlib/contrib/qxip/clickhouse/clickhouse.flux Outdated Show resolved Hide resolved
stdlib/contrib/qxip/clickhouse/clickhouse.flux Outdated Show resolved Hide resolved
stdlib/contrib/qxip/clickhouse/clickhouse.flux Outdated Show resolved Hide resolved
stdlib/contrib/qxip/clickhouse/clickhouse.flux Outdated Show resolved Hide resolved
stdlib/contrib/qxip/clickhouse/clickhouse.flux Outdated Show resolved Hide resolved
@lmangani
Copy link
Contributor Author

lmangani commented Dec 20, 2022

I think you may also need to run make generate?

Thanks @wolffcm for pointing this out. I've updated the PR accordingly and learned a good lesson for the next.
Thanks @sanderson for the patient review and the improvements!

@lmangani
Copy link
Contributor Author

lmangani commented Dec 20, 2022

EDIT: test failing in the new edits

error contrib/qxip/clickhouse/clickhouse.flux@40:1-64:6: missing documentation for parameter "cors"
This error was addressed

error contrib/qxip/clickhouse/clickhouse.flux@13:1-13:44: value defaultURL must contain a non empty comment
This error is unclear to me and I couldn't find a fitting example so far

Copy link
Contributor

@sanderson sanderson left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for this!

@wolffcm
Copy link

wolffcm commented Dec 21, 2022

@lmangani Again thanks for this contribution. There is just one CI issue left. I don't know if you can see it. CI is complaining about "conventional commits." Basically, the message for each commit needs to follow a standardized format. It helps us automate the generation of changelogs and such.

The description of the format is here:
https://www.conventionalcommits.org/en/v1.0.0/#summary

The title of this PR feat(contrib/qxip): add contrib/qxip/clickhouse package would be a fine commit message, so if you were to squash all 22 commits to a single commit with that message and force push your branch, that would make CI happy.

If you want to go ahead and do this, I will approve and merge to master immediately (provided it's during my workday).

@lmangani
Copy link
Contributor Author

The title of this PR feat(contrib/qxip): add contrib/qxip/clickhouse package would be a fine commit message, so if you were to squash all 22 commits to a single commit with that message and force push your branch, that would make CI happy.

In case this works out, and for any future visitor in the same predicament (including myself) I've used the following commands from the PR/branch:

git reset --soft master
git add .
git commit -m "feat(contrib): adding a proper commit message"
git push -f

@wolffcm wolffcm merged commit 502babd into influxdata:master Dec 21, 2022
@lmangani
Copy link
Contributor Author

lmangani commented Dec 21, 2022

Thanks @wolffcm and @sanderson for your patient assistance and happy holidays to the Flux team!

         v
        >X<
         A
        d$b
      .d\$$b.
    .d$i$$\$$b.
       d$$@b
      d\$$$ib
    .d$$$\$$$b
  .d$$@$$$$\$$ib.
      d$$i$$b
     d\$$$$@$b
  .d$@$$\$$$$$@b.
.d$$$$i$$$\$$$$$$b.
        ###
        ###
        ### 

See on in the next PR and/or next year!

@lmangani lmangani deleted the contrib-clickhouse branch January 3, 2023 21:52
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.

3 participants