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

Use internal Volume struct for tls #578

Conversation

bshephar
Copy link
Contributor

@bshephar bshephar commented Nov 6, 2024

This change switches the tls package over to using the lib-common copy of the Volume struct. This will allow us to append TLS volumes to the volume struct in each service Operator as we update the version of lib-common in each of them.

@dprince
Copy link
Contributor

dprince commented Nov 6, 2024

seems good to me

@bshephar bshephar force-pushed the tls-internal-storage-struct branch from 21c6972 to bd2119d Compare November 6, 2024 02:16
This change switches the tls package over to using the lib-common
copy of the Volume struct. This will allow us to append TLS volumes to the volume
struct in each service Operator as we update the version of lib-common in each of
them.

Signed-off-by: Brendan Shephard <[email protected]>
@stuggi
Copy link
Contributor

stuggi commented Nov 6, 2024

I am not sure I understand the intend for this switch. the tls volumes are appended to the deployment volumes in the operators, so doesn't this change just required another conversion to the k8s volume?

@fmount
Copy link
Contributor

fmount commented Nov 6, 2024

-1 to switch to storage.Volume instead of simply using corev1.Volume. We don't want to be in the business of doing a lot of conversions, and we already "convert" to corev1.Volume what we have with extraMounts.
Is there a particular reason that led to this change?

@fmount fmount self-requested a review November 6, 2024 07:59
@bshephar
Copy link
Contributor Author

bshephar commented Nov 6, 2024

Yeah, my bad. This isnt' necessary

@bshephar bshephar closed this Nov 6, 2024
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.

4 participants