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

WIP: Proposal for new values shape #1191

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

chrisseto
Copy link
Contributor

@chrisseto chrisseto commented Apr 12, 2024

Early working draft of my proposal on where we should move with our Values struct. Currently focused on redpanda.yaml and providing an "advanced mode" type of experience.

This PR is for discussion and will likely be splintered across many PRs once there's a general consensus.

Various notes:

  • Going to use kubebuilder annotations so this struct is "CRD Ready"
  • JSONSchema generation is a bit cumbersome and would duplicate a lot of kubebuilder annotations. Looking at writing something to generate JSONSchema from kubebuilder annotations. Using controller-gen and then converting the CRD to JSONSchema is placing too much load on 3rd party tooling.
  • Trying to build out documentation scaffolding within the struct itself in a way that's useful for developers and end users by using an explicit --- divider.
  • Generally following CRD practices where "One Ofs" or Union Types are expressed as a struct with many nullable keys.
  • Focusing VERY intensely on the exact semantics of zero values, partial values, etc.
  • This is a "bottom level" abstraction. It should provide minimal helpers and always allow direct access in some way. Other abstractions will talk in terms of this abstraction.
  • Documentation should lean on other forms of existing documentation rather than risk becoming stale and needing to be kept in sync.

@@ -62,3 +62,114 @@ func RedpandaAdditionalStartFlags(dot *helmette.Dot, smp, memory, reserveMemory

return append(flags, values.Statefulset.AdditionalRedpandaCmdFlags...)
}

// func RedpandaYAMLKafkaListeners(dot *helmette.Dot) []KafkaListener {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was exploring the existing listener configurations. Will likely have a function that converts the existing shape into this structure and we'll convert our templates to use the new structure.

@@ -1,12 +1,15 @@
//+gotohelm:ignore=true
// +gotohelm:ignore=true
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I wonder why my IDE is changing this every time 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually due to go fmt. We don't run it against the entire codebase right now and this annotation is technically incorrect 😓 . Filed an issue #1194

Comment on lines +83 to +95
// KeyFile maps to the `kafka_api_tls[*].key_file` field.
KeyFile FileSource

// CertFile maps to the `kafka_api_tls[*].cert_file` field.
CertFile FileSource

// TrustStoreFile maps to the `kafka_api_tls[*].truststore_file` field.
// +kubebuilder:default={"path": "/etc/ssl/certs/ca-certificates.crt"}
TrustStoreFile *FileSource
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how users would control volume mount points.

Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

I like this approach. One requirement from me, that would defiantly land in core team backlog, is node configuration schema. I see potential problems with Redpanda core having fields deprecation that affects our code. That breaking change never happen (as far as I remember), so I would vote for this change/approach 👍

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