OCM-5677 | feat: Added `rosa describe autoscaler` command #1837

robpblake · 2024-03-12T15:25:25Z

This PR adds rosa describe autoscaler to the ROSA CLI to allow customers to view details of their cluster autoscaler.

This PR attempts to introduced some of the concepts from the ROSA unit testing DDR to explore how we can make our commands more test friendly.

robpblake · 2024-03-12T15:32:08Z

/remove-approve

codecov · 2024-03-12T16:20:31Z

Codecov Report

Attention: Patch coverage is 80.66667% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 20.92%. Comparing base (b049fc4) to head (4c54777).
Report is 6 commits behind head on master.

Files	Patch %	Lines
pkg/rosa/runner.go	36.36%	13 Missing and 1 partial ⚠️
pkg/ocm/output/nodepools.go	0.00%	5 Missing ⚠️
cmd/describe/cluster/cmd.go	0.00%	3 Missing ⚠️
pkg/ocm/output/machinepools.go	0.00%	3 Missing ⚠️
pkg/output/output.go	0.00%	3 Missing ⚠️
pkg/ocm/flag.go	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #1837     +/-   ##
=========================================
  Coverage   20.91%   20.92%             
=========================================
  Files         103      113     +10     
  Lines       17351    18662   +1311     
=========================================
+ Hits         3629     3905    +276     
- Misses      13447    14470   +1023     
- Partials      275      287     +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gdbranco · 2024-03-12T20:37:38Z

pkg/clusterautoscaler/validation.go

+
+func IsAutoscalerSupported(runtime *rosa.Runtime, cluster *cmv1.Cluster) error {
+	if cluster.Hypershift().Enabled() {
+		return fmt.Errorf(NoHCPAutoscalerSupportMessage)


Autoscaler is supported on HCP, but is only enabled for IBM if I'm not mistaken

@gdbranco I replicated the logic from create autoscaler which doesn't seem to do any check to see if current org has capability to create autoscaler for their HCP cluster. IIRC, IBM doesn't use the CLI, but OCM SDK directly?

Correct, they are using SDK from what I know

robpblake · 2024-03-14T14:26:58Z

cmd/describe/autoscaler/cmd.go

+		err = clusterautoscaler.IsAutoscalerSupported(runtime, cluster)
+		if err != nil {
+			return err
+		}
+
+		autoscaler, err := runtime.OCMClient.GetClusterAutoscaler(cluster.ID())
+		if err != nil {
+			return err
+		}
+
+		if autoscaler == nil {
+			return fmt.Errorf("No autoscaler exists for cluster '%s'", runtime.ClusterKey)
+		}


Does it make sense to decompose this further into a new ClusterAutoscalerService so that the command becomes even briefer e.g:

service := NewClusterAutoscalerService(runtime.OCMClient) supported, err = service.IsAutoscalerSupported(runtime, cluster) autoscaler, err := service.GetClusterAutoscaler(cluster.ID()) if err != nil { return err }

I think it's ok to keep it that way, it seems like we return nil for StatusNotFound, and have different error messages per command.
I wish all the commands in ROSA were as brief as this one:)

With that being said, we could return a boolean from the function for exist and check the boolean value instead of nil to improve readability (maybe outside of the scope of this PR).

robpblake · 2024-03-14T17:10:56Z

cmd/describe/autoscaler/cmd.go

+rosa describe autoscaler --cluster foo`
+)
+
+func NewDescribeAutoscalerCommand() *cobra.Command {


The command is returned, fully initialised from a function. There is no use of global variables (ignoring the output.AddFlag and ocm.AddClusterFlag calls which are beyond the scope of this particular refactor). There is no use of init

robpblake · 2024-03-14T17:12:07Z

cmd/describe/autoscaler/cmd.go

+	return cmd
+}
+
+func DescribeAutoscalerRunner() rosa.CommandRunner {


Command logic is executed by a CommandRunner. The runner does not os.Exit, but instead returns an error if something went wrong. This makes it fully testable. The passed Runtime can be instantiated with mocks in the tests.

robpblake · 2024-03-14T17:13:56Z

pkg/rosa/runner.go

+
+// DefaultRunner is a centralised implementation of the default Cobra Command.run function that takes care
+// of instantiating several key resources on behalf of a command
+func DefaultRunner(visitor RuntimeVisitor, runner CommandRunner) func(command *cobra.Command, args []string) {


Centralised handling of Context and Runtime instantiation. Also centralised the handling of the deferred r.Cleanup() (Need to further discuss if we actually need this as the runtime exits at the end of the execution.)

oriAdler

Nice approach!
I wonder how complicated would it be for interactive commands.
Is it worth going through the PR during one of the office hours? to enhance understanding and share with the team?

cmd/describe/autoscaler/cmd_test.go

pkg/ocm/flag.go

pkg/rosa/runner.go

robpblake · 2024-03-21T13:53:16Z

cmd/upgrade/accountroles/cmd.go

@@ -242,10 +242,8 @@ func run(cmd *cobra.Command, _ []string) {
 	case aws.ModeAuto:
 		if isUpgradeNeedForAccountRolePolicies {
 			reporter.Infof("Starting to upgrade the policies")
-			err = r.WithSpinner(func() error {


Our AWS SDK v2 rebase incorrectly bought this back in, so just returning us to previous state.

ciaranRoche · 2024-03-22T14:27:58Z

LGTM, happy to merge this if we want. I think the overall struct is good, and small improvements can be followed up as we tackle a more complex command next
WDYT?

robpblake · 2024-03-22T14:31:17Z

LGTM, happy to merge this if we want. I think the overall struct is good, and small improvements can be followed up as we tackle a more complex command next WDYT?

@ciaranRoche That makes sense to me. +1

ciaranRoche · 2024-03-25T10:41:35Z

/lgtm

openshift-ci · 2024-03-25T10:42:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ciaranRoche, oriAdler

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:

~~OWNERS~~ [ciaranRoche,oriAdler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-03-25T10:56:54Z

@robpblake: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci bot requested review from den-rgb and oriAdler March 12, 2024 15:27

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2024

openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2024

robpblake force-pushed the ocm-5677-view-autoscaler branch from d2aafc3 to c6eede7 Compare March 12, 2024 16:12

gdbranco reviewed Mar 12, 2024

View reviewed changes

robpblake mentioned this pull request Mar 13, 2024

OCM-6527 | feat: add describe ingress command #1841

Merged

robpblake force-pushed the ocm-5677-view-autoscaler branch 2 times, most recently from b1f8b38 to f421090 Compare March 13, 2024 17:09

robpblake commented Mar 14, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2024

robpblake commented Mar 14, 2024

View reviewed changes

oriAdler reviewed Mar 19, 2024

View reviewed changes

pkg/rosa/runner.go Outdated Show resolved Hide resolved

robpblake force-pushed the ocm-5677-view-autoscaler branch from f421090 to 47a0009 Compare March 21, 2024 12:29

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2024

robpblake commented Mar 21, 2024

View reviewed changes

OCM-5677 | feat: Added rosa describe autoscaler command

4c54777

robpblake force-pushed the ocm-5677-view-autoscaler branch from 2b5b12b to 4c54777 Compare March 21, 2024 15:39

oriAdler approved these changes Mar 24, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2024

openshift-ci bot assigned ciaranRoche Mar 25, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2024

openshift-merge-bot bot merged commit 68ce5a0 into openshift:master Mar 25, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCM-5677 | feat: Added `rosa describe autoscaler` command #1837

OCM-5677 | feat: Added `rosa describe autoscaler` command #1837

robpblake commented Mar 12, 2024

robpblake commented Mar 12, 2024

codecov bot commented Mar 12, 2024 •

edited

Loading

gdbranco Mar 12, 2024

robpblake Mar 13, 2024 •

edited

Loading

gdbranco Mar 13, 2024

robpblake Mar 14, 2024

oriAdler Mar 19, 2024

robpblake Mar 14, 2024

robpblake Mar 14, 2024

robpblake Mar 14, 2024 •

edited

Loading

oriAdler left a comment

robpblake Mar 21, 2024

ciaranRoche commented Mar 22, 2024

robpblake commented Mar 22, 2024

ciaranRoche commented Mar 25, 2024

openshift-ci bot commented Mar 25, 2024

openshift-ci bot commented Mar 25, 2024

OCM-5677 | feat: Added rosa describe autoscaler command #1837

OCM-5677 | feat: Added rosa describe autoscaler command #1837

Conversation

robpblake commented Mar 12, 2024

robpblake commented Mar 12, 2024

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

gdbranco Mar 12, 2024

Choose a reason for hiding this comment

robpblake Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

gdbranco Mar 13, 2024

Choose a reason for hiding this comment

robpblake Mar 14, 2024

Choose a reason for hiding this comment

oriAdler Mar 19, 2024

Choose a reason for hiding this comment

robpblake Mar 14, 2024

Choose a reason for hiding this comment

robpblake Mar 14, 2024

Choose a reason for hiding this comment

robpblake Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

oriAdler left a comment

Choose a reason for hiding this comment

robpblake Mar 21, 2024

Choose a reason for hiding this comment

ciaranRoche commented Mar 22, 2024

robpblake commented Mar 22, 2024

ciaranRoche commented Mar 25, 2024

openshift-ci bot commented Mar 25, 2024

openshift-ci bot commented Mar 25, 2024

OCM-5677 | feat: Added `rosa describe autoscaler` command #1837

OCM-5677 | feat: Added `rosa describe autoscaler` command #1837

codecov bot commented Mar 12, 2024 •

edited

Loading

robpblake Mar 13, 2024 •

edited

Loading

robpblake Mar 14, 2024 •

edited

Loading