refactor(api)!: make Kubebuilder types protobuf compatible #1515
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 7 commits
February 21, 2024 02:15
This commit adds a `deps-tools` target to Makefile to manage tools that should be managed with Go. It leverages `go.mod` to pin the version of tools. Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
✅ Deploy Preview for docs-kargo-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
added 4 commits
February 22, 2024 15:02
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
devholic
force-pushed
the
devholic/#708
branch
2 times, most recently
from
b03b4c8 to
f90390f
Compare
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
devholic
force-pushed
the
devholic/#708
branch
from
f90390f to
7e79821
Compare
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
hiddeco
reviewed
Feb 22, 2024
krancour
changed the title
krancour
reviewed
Feb 22, 2024
krancour
reviewed
Feb 22, 2024
krancour
reviewed
Feb 22, 2024
|
@devholic this is awesome work! I'm fine with everything I see here, but still want to test-drive it just to make sure I really understand how the new codegen process feels. I will do on Tuesday. I also just want to check with @jessesuen and @gdsoumya that they have no specific objections to using only binary-encoded protobufs. |
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1515 +/- ##
==========================================
- Coverage 30.87% 21.64% -9.24%
==========================================
Files 188 187 -1
Lines 20162 28040 +7878
==========================================
- Hits 6226 6068 -158
- Misses 13652 21701 +8049
+ Partials 284 271 -13 ☔ View full report in Codecov by Sentry. |
|
When I test-drive this new codegen process using I know not everyone is as keen as I am about isolating work like this to a container, but especially given the large number of tools involved in code generation, I think it's very important that this work. |
|
I am also not seeing the |
added 3 commits
March 8, 2024 02:49
#1515 (comment) Signed-off-by: Sunghoon Kang <hoon@akuity.io>
#1515 (comment) Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
I tried to fix it by giving dev-tools user permission on
Fixed in 8adde4e |
|
@devholic this works perfectly! Slower than the old process, but a very small price to pay for no longer having to write type conversions by hand! 🔥 🔥 🔥 |
krancour
approved these changes
Mar 7, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
Signed-off-by: Sunghoon Kang <hoon@akuity.io>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This was referenced Mar 12, 2024
devholic
pushed a commit
that referenced
this pull request
Apr 2, 2024
`intstr.IntOrString` relies on `gogo/protobuf` for JSON encoding/decoding, which is incompatible with the protobuf dependency used in our UI. This commit extends `IntOrString` to encode/decode to/from JSON value to make compatible with `gogo/protobuf`. Related: - #708 - #1515 Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #708
🚨 THIS PR CONTAINS BREAKING CHANGES (NON-BACKWARD COMPATIBLE) 🚨
Background
Current implementation of converting between Go types (Kubebuilder) and Protobuf types (API Request / Response) may cause unintended bugs since the conversion code should be generated by hand.
This PR removes hand-written type conversion code by making Kubebuilder types protobuf compatible.
Changes
Now
codegentarget will generate intermediate protobuf definition from the Kubebuilder types using go-to-protobuf (part of kubernetes/code-generator).Since we assume Kubebuilder structs as a source of truth, we will not use generated protobuf code from above. Instead,
hack/codegen/prototagwill extractprotobuftag from the generated code then inject into the Kubebuilder structs. This will help us not to write/manageprotobuftags.Limitations
jsonsupportgogo/protobufextendsgoogle/protobufto support various features. However these extended feature doesn't work on cross-platform environment - for example, json encoding/decoding for some messages (e.g. metav1.Time,one_ofmessage) fails between API and UI.To make things simple, this PR makes binary-encoded protobuf as a first-class citizen, and drops
jsonsupport from API for the consistency.