Skip to content

feat: Remove reflect methods#2129

Merged
kodiakhq[bot] merged 17 commits intomainfrom
remove-reflect-methods
May 19, 2025
Merged

feat: Remove reflect methods#2129
kodiakhq[bot] merged 17 commits intomainfrom
remove-reflect-methods

Conversation

@bbernays
Copy link
Copy Markdown
Contributor

@bbernays bbernays commented Apr 24, 2025

Summary

Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@bbernays bbernays requested review from a team and jon-s58 April 24, 2025 14:26
@github-actions github-actions bot added the feat label Apr 24, 2025
@bbernays bbernays changed the title feat: Remove reflect methods feat!: Remove reflect methods Apr 24, 2025
erezrokah
erezrokah reviewed Apr 28, 2025
View reviewed changes
erezrokah
erezrokah reviewed Apr 28, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Maybe we can bring back #1142? That should resolve https://github.com/cloudquery/cloudquery-issues/issues/2500

@bbernays
Copy link
Copy Markdown
Contributor Author

bbernays commented Apr 28, 2025

Maybe we can bring back #1142? That should resolve cloudquery/cloudquery-issues#2500

I have no problem with this

erezrokah
erezrokah requested changes May 9, 2025
View reviewed changes
erezrokah
erezrokah approved these changes May 16, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We'll need to do a follow up in the FIPS plugins but I think that's fine.
One thing I'm not sure about is classifying this as a breaking change, that would require a bit of work for our plugins to do the major version bump. The potential breaking change here is the grpcnotrace flag, which plugins authors can override now

@bbernays
Copy link
Copy Markdown
Contributor Author

bbernays commented May 16, 2025 •
edited
Loading

We'll need to do a follow up in the FIPS plugins but I think that's fine. One thing I'm not sure about is classifying this as a breaking change, that would require a bit of work for our plugins to do the major version bump. The potential breaking change here is the grpcnotrace flag, which plugins authors can override now

I am fine making it a non-breaking change, but the reason why it is breaking change is because it removes the ability for the plugin to generate markdown docs...

plugin-sdk/serve/docs.go

Lines 46 to 51 in 948fce1

g := docs.NewGenerator(s.plugin.Name(), tables)
if format.Value != "json" {
return errors.New("only json format is supported. If need to generate markdown, use the `cloudquery tables` command")
}
return g.GenerateJSON(args[0], docs.FormatJSON)

@erezrokah
Copy link
Copy Markdown
Member

erezrokah commented May 16, 2025

but the reason why it is breaking change is because it removes the ability for the plugin to generate markdown docs...

Ah yeah that hasn't been used for a long time I believe and we were the only users

@bbernays
Copy link
Copy Markdown
Contributor Author

bbernays commented May 16, 2025

So just to be clear you are good with me marking this as a non breaking change?

cc @murarustefaan

@erezrokah
Copy link
Copy Markdown
Member

erezrokah commented May 16, 2025

BTW #1142 removes docs generation altogether, do we need JSON docs? Can be done in another PR

@murarustefaan
Copy link
Copy Markdown
Member

murarustefaan commented May 16, 2025

So just to be clear you are good with me marking this as a non breaking change?

Yes, I do not think the potential impact this change has warrants a major SDK version bump.

... do we need JSON docs? ...

Nope. But since the changeset for removing doc generation completely is bigger, we'd be better off having that in a separate PR.

@murarustefaan murarustefaan changed the title feat!: Remove reflect methods feat: Remove reflect methods May 16, 2025
@kodiakhq kodiakhq bot merged commit bd277cc into main May 19, 2025
10 checks passed
@kodiakhq kodiakhq bot deleted the remove-reflect-methods branch May 19, 2025 13:41
@cq-bot cq-bot mentioned this pull request May 19, 2025
Merged
kodiakhq bot pushed a commit that referenced this pull request May 19, 2025
@cq-bot
chore(main): Release v4.81.0 (#2159)
47d28e0 
🤖 I have created a release *beep* *boop*
---


## [4.81.0](v4.80.3...v4.81.0) (2025-05-19)


### Features

* Remove reflect methods ([#2129](#2129)) ([bd277cc](bd277cc))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@murarustefaan murarustefaan murarustefaan approved these changes

@erezrokah erezrokah erezrokah approved these changes

@jon-s58 jon-s58 Awaiting requested review from jon-s58

+1 more reviewer

@savme savme savme approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

automerge feat

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bbernays @erezrokah @murarustefaan @savme