Embed struct, not pointer in resources #2829
Merged
Conversation
This file contains hidden or 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
denik
changed the title
denik
requested review from
andrewnester,
anton-107,
pietern and
shreyas-goenka
as code owners
anton-107
approved these changes
May 8, 2025
andrewnester
approved these changes
May 8, 2025
… commit message that captures the essence of the transformation: refactor: replace App pointer embedding with direct struct embedding
This commit resolves type mismatch errors in test files by converting `compute.ClusterSpec{}` to `&compute.ClusterSpec{}` to match the expected pointer type.
The changes affect multiple test files in the bundle configuration and deployment packages, ensuring consistent type usage for cluster specifications.
This reverts commit 8bd4ddb.
denik
force-pushed
the
denik/embed-struct-not-ptr
branch
from
a1add45 to
9fa0be8
Compare
pietern
reviewed
May 9, 2025
Contributor
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
Thanks, great simplification!
|
|
||
| assert.Nil(t, b.Config.Resources.Volumes["nilVolume"]) | ||
| assert.Nil(t, b.Config.Resources.Volumes["emptyVolume"].CreateVolumeRequestContent) | ||
| // assert.Nil(t, b.Config.Resources.Volumes["emptyVolume"].CreateVolumeRequestContent) |
Contributor
Author
There was a problem hiding this comment.
Not a dup (second line used to check inner pointer), should have been cleaned up.
| ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ | ||
| "my_model_serving_endpoint": { | ||
| CreateServingEndpoint: &serving.CreateServingEndpoint{}, | ||
| CreateServingEndpoint: serving.CreateServingEndpoint{}, |
Contributor
There was a problem hiding this comment.
FWIW, with these structs being zero-initialized, the assignment can be removed entirely.
Contributor
Author
There was a problem hiding this comment.
I agree. For context, most replacement were made with search-and-replace, so they look odd (AI tools were going back and forth on whether it should be a pointer or a struct, so I stopped using those for this).
I feel like there should be a linter to clean this up, but I could not find one.
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.
Changes
Why
Tests
Existing tests.