Add warning to deprecate disableCSI through CLI #5918
Conversation
eks-distro-bot
added
the
size/S
Codecov Report
@@ Coverage Diff @@
## main #5918 +/- ##
=======================================
Coverage 73.66% 73.67%
=======================================
Files 452 452
Lines 37887 37895 +8
=======================================
+ Hits 27911 27919 +8
Misses 8359 8359
Partials 1617 1617
|
| func warnIfCSIEnabled(disableCSI bool) { | ||
| if !disableCSI { | ||
| // Need to add spacing to make the log message look neat | ||
| logger.MarkWarning(" Warning: Installing CSI through EKS Anywhere is deprecated. Refer to the official documentation for more details on " + |
There was a problem hiding this comment.
I think that wording is fine. Saying that something "is deprecated" means that it will be going away in the future, but has not gone away yet.
There was a problem hiding this comment.
@vivek-koppuru Do we have a ticket users can track? Its useful to include that in warnings. No worries if not.
There was a problem hiding this comment.
We have this issue #5517 but if we feel that is not the right one, we can create a different one. I am still leaning towards putting it in our documentation and changelog and not here because I don't want to make the message bigger, unless we think that is not a problem.
|
/lgtm |
I plan on updating the documentation but I didn't want to link it because the log message would get more verbose than it already is. |
That makes sense. I think it's fine. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vivek-koppuru The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue #, if available:
#5517
Description of changes:
Adding a warning that this field and functionality will not be supporting starting from the next release. Making this the first warning that appears when running the CLI for create and upgrade if
disableCSIis set tofalse, which means that the user expects to have the vsphere csi driver and storage class installed on the cluster. We have documentation today on what it entails to disable it and we will add more information to the docs about this deprecation: https://anywhere.eks.amazonaws.com/docs/reference/clusterspec/vsphere/#disablecsi-optionalOutput:
I looked into how to add a warning to the webhook when creating workload clusters through kubectl, and it doesn't seem to be straightforward. Openapi supports adding the deprecated field, but I can't find how to enable that through Kubebuilder except for
kubebuilder:deprecatedversion, which deprecates the whole api. I looked into seeing if we can throw the warning through the Validating webhook to have the warning displayed, but it looks like it was just merged recently here. Our best option here instead of trying to find a different way to do it is to just throw the warning through the CLI and omit the warning for the controller, as this message will be displayed first to users through the CLI anyways when they create/upgrade their management clusters.Testing (if applicable):
unit testing and functional testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.