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

Remove unnecessary config marshalling wherever possible #2735

Conversation

jaronoff97
Copy link
Contributor

Description:

Link to tracking Issue(s):

  • Resolves: #issue-number

Testing:

Documentation:

@@ -131,57 +137,40 @@ func TestConfigToProbeShouldErrorIf(t *testing.T) {
service:
extensions: [health_check]`,
expectedErr: errNoExtensionHealthCheck,
}, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some of these tests are no longer applicable!

@@ -98,14 +98,56 @@ func (c Config) Yaml() (string, error) {
return buf.String(), nil
}

// MetricsConfig comes from the collector
type MetricsConfig struct {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be introduced in a separate PR?

type Service struct {
Extensions *[]string `json:"extensions,omitempty" yaml:"extensions,omitempty"`
Extensions []string `json:"extensions,omitempty" yaml:"extensions,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

IIRC this will enforce marshalling of empty array for extensions. Is this necessary?

// +kubebuilder:pruning:PreserveUnknownFields
Telemetry *AnyConfig `json:"telemetry,omitempty" yaml:"telemetry,omitempty"`
// +kubebuilder:pruning:PreserveUnknownFields
Pipelines AnyConfig `json:"pipelines" yaml:"pipelines"`
}

// GetTelemetry serves as a helper function to access the fields we care about in the underlying telemetry struct.
// This exists to avoid needing to worry extra fields in the telemetry struct.
func (s *Service) GetTelemetry() *Telemetry {
Copy link
Member

Choose a reason for hiding this comment

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

could we add a test for this? Getting a telemetry from an existing yaml?

@jaronoff97
Copy link
Contributor Author

closing in favor of #2857

@jaronoff97 jaronoff97 closed this Apr 16, 2024
@jaronoff97 jaronoff97 deleted the 2603-remove-unnecessary-config-marshalling branch April 16, 2024 20:49
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