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

Use any instead of interface{} #984

Merged
merged 5 commits into from
May 5, 2023
Merged

Conversation

candiduslynx
Copy link
Contributor

@candiduslynx candiduslynx commented May 2, 2023

Summary

Go 1.18 (that is targeted by go.mod) introduced a handy any alias for interface{}, so use it for reading & dev simplicity.

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG
  • For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

@candiduslynx
Copy link
Contributor Author

Hi @alexey-milovidov!
Is there any chance to get this reviewed?

@jkaflik
Copy link
Contributor

jkaflik commented May 4, 2023

@candiduslynx I will review it once have checks green.

@jkaflik jkaflik self-requested a review May 4, 2023 09:01
@candiduslynx
Copy link
Contributor Author

@jkaflik It seems that the failing test is flaky (failed to dial?) Could you trigger the rerun of failed jobs?

@jkaflik
Copy link
Contributor

jkaflik commented May 4, 2023

@candiduslynx given our workflow setup, it's expected to fail on the fork run. Please do not worry about it.

@candiduslynx
Copy link
Contributor Author

@jkaflik
Ah, great. All others are green, so I presume you'll be taking this onto review, right?

Copy link
Contributor

@jkaflik jkaflik left a comment

Choose a reason for hiding this comment

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

LGTM. Staging for a release

@candiduslynx
Copy link
Contributor Author

BTW, I didn't include a lot of the linters (like, errcheck) because it highlights a lot of issues, and I feel like you'll need to address those specifically (remove the disable all, add other linters. Example cfg we have in CQ SDK: https://github.com/cloudquery/plugin-sdk/blob/main/.golangci.yml)

@candiduslynx
Copy link
Contributor Author

@jkaflik could you point me to some doc or walk through the RC process for SDK? Are you performing some internal testing to ensure consistency and then merge manually? Or is there something you tweak and then the -cloud jobs should pass?

(just curios)

@jkaflik
Copy link
Contributor

jkaflik commented May 5, 2023

@candiduslynx, we don't have (yet) a formalized release process for SDKs. Currently, I stage non-critical PRs with a label and merge them before major/minor release.

Issue with running tests against cloud on fork PRs can be tracked here.

@candiduslynx
Copy link
Contributor Author

Currently, I stage non-critical PRs with a label and merge them before major/minor release.

@jkaflik thanks for explaining!

Would you consider merging this earlier, as this includes linter rules to ensure any instead of interface{} and postponing might introduce conflicts to be resolved (basically, someone adding more code with interface{})?

@jkaflik jkaflik changed the title feat: Use any instead of interface{} Use any instead of interface{} May 5, 2023
@jkaflik jkaflik merged commit eec85ec into ClickHouse:main May 5, 2023
@candiduslynx candiduslynx deleted the feat/any branch May 6, 2023 06:19
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