Update CLI logging output to be cluster type aware: #8231
Conversation
eks-distro-bot
added
the
size/S
jacobweinstock
force-pushed
the
update-cli-logging
branch
from
7cec6f0 to
acba587
Compare
We have been leaking implementation details around how we create clusters. While CAPI calls the initial cluster a workload cluster and then pivots its control plane objects to make it a management cluster this is an implementation detail that we shouldn't be exposing to users. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>
jacobweinstock
force-pushed
the
update-cli-logging
branch
from
acba587 to
506883b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8231 +/- ##
=======================================
Coverage 73.42% 73.42%
=======================================
Files 577 577
Lines 35944 35944
=======================================
Hits 26391 26391
Misses 7884 7884
Partials 1669 1669 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>
This func is only for management cluster anyway, so there is no need to check if its a management cluster or not. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>
|
/test eks-anywhere-presubmit |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jacobweinstock 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 |
| @@ -47,7 +47,7 @@ func (s *installEksaComponentsOnWorkloadTask) Run(ctx context.Context, commandCo | |||
| commandContext.ClusterSpec.Cluster.ClearTinkerbellIPAnnotation() | |||
| } | |||
|
|
|||
| logger.Info("Installing EKS-A custom components on workload cluster") | |||
| logger.Info("Installing EKS-A custom components on the management cluster") | |||
There was a problem hiding this comment.
nit: initially the workload cluster in all these places meant wrt the kind cluster being the mgmt cluster vs the workload cluster getting created by the kind cluster.
Looks fine to me if we want to make this change tho since by this time we already would've moved the capi components.
There was a problem hiding this comment.
I don't believe a new customer coming to eksa for the first time is going to grasp this implementation detail. They have asked for a management cluster and are now seeing logs about a workload cluster.
There was a problem hiding this comment.
Think of the cognitive load we're requiring from a customers just to understand this one log line. They have to understand quite a lot of implementation details of how we create clusters. I believe that's an unreasonable expectation.
There was a problem hiding this comment.
Yeah it looks fine to me since for the customer can understand this better. When we use this statement for debugging the engineer can probably figure out which cluster we are talking about. LGTM!
Issue #, if available:
Description of changes:
This removes the confusion in the CLI logs when creating a management cluster as the log lines say it's creating a workload cluster. This is a leak of implementation details about how clusters are created. While CAPI calls the initial cluster a workload cluster and then pivots its control plane objects to make it a permanent management cluster this is an implementation detail we shouldn't expose to users.
Testing (if applicable):
Documentation added/planned (if applicable):
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.