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

OCM-8152 | test: automated id:38816 List cluster ROSA cli will work #2076

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AkashKanni
Copy link
Contributor

OCM-8152 QE] Automate OCP-38816 [rosacli] List cluster via ROSA cli will work well

$ ginkgo --focus 38816 tests/e2e
Ginkgo detected a version mismatch between the Ginkgo CLI and the version of Ginkgo imported by your packages:
Ginkgo CLI Version:
2.11.0
Mismatched package versions found:
2.17.1 used by e2e

Ginkgo will continue to attempt to run but you may see errors (including flag
parsing errors) and should either update your go.mod or your version of the
Ginkgo CLI to match.

To install the matching version of the CLI run
go install github.com/onsi/ginkgo/v2/ginkgo
from a path that contains a go.mod file. Alternatively you can use
go run github.com/onsi/ginkgo/v2/ginkgo
from a path that contains a go.mod file to invoke the matching version of the
Ginkgo CLI.

If you are attempting to test multiple packages that each have a different
version of the Ginkgo library with a single Ginkgo CLI that is currently
unsupported.

Running Suite: e2e tests suite - /home/akanni/OCP-Repository/rosa/tests/e2e

Random Seed: 1716211140

Will run 1 of 76 specs
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS•SSSSSSSSSSSSSSSSSSSSS

Ran 1 of 76 Specs in 5.456 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 75 Skipped
PASS

Ginkgo ran 1 suite in 7.115914938s
Test Suite Passed

Copy link

codecov bot commented May 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 26.42%. Comparing base (a455467) to head (76dd563).
Report is 793 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2076   +/-   ##
=======================================
  Coverage   26.42%   26.42%           
=======================================
  Files         150      150           
  Lines       21294    21294           
=======================================
  Hits         5626     5626           
  Misses      15244    15244           
  Partials      424      424           

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

@@ -55,6 +55,7 @@ type OCMResourceService interface {

ListOIDCConfig() (OIDCConfigList, bytes.Buffer, error)
ListInstanceTypes() (InstanceTypesList, bytes.Buffer, error)
ListClusters(flags ...string) (ClusterList, bytes.Buffer, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not reusing existing/extending method in Cluster service ?
https://github.com/openshift/rosa/blob/master/tests/utils/exec/rosacli/cluster_service.go#L21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I wasn't aware about this
i have pushed the fix, could you please take a look? Thanks!

Copy link
Contributor

openshift-ci bot commented May 21, 2024

@AkashKanni: 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-sigs/prow repository. I understand the commands that are listed here.

@@ -18,7 +18,7 @@ type ClusterService interface {
DescribeCluster(clusterID string) (bytes.Buffer, error)
ReflectClusterDescription(result bytes.Buffer) (*ClusterDescription, error)
DescribeClusterAndReflect(clusterID string) (*ClusterDescription, error)
List() (bytes.Buffer, error)
ListCluster(flags ...string) (ListCluster, bytes.Buffer, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not add yet another return type. It is better to have a ReflectClusterList like we have for others

Copy link
Contributor Author

@AkashKanni AkashKanni Jul 4, 2024

Choose a reason for hiding this comment

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

Hi @radtriste
All others are returning output in form of byte and their responses don't looks like in form of columns, But for rosa list cluster it would be efficient to return the data the structure (column wise) format in this way we can easily use the values ( ID, NAME, STATE, etc) easily in automation code.

Copy link
Contributor

Choose a reason for hiding this comment

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

@AkashKanni I was meaning to have a ReflectClusterList method similar to ReflectClusterDescription which is taking the bugger result and parsing it into struct

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update following this ?

var _ = Describe("List resources",
labels.Day2,
labels.FeatureMachinepool,
labels.NonHCPCluster,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this label ? This should work also for HCP or ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@AkashKanni Please answer to this ?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 4, 2024
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

@ciaranRoche
Copy link
Member

@AkashKanni what is the current status of this MR, is it still valid or can it be closed?

@AkashKanni
Copy link
Contributor Author

@ciaranRoche Its a long due PR, its a valid one and i will try to resolve the comments and get it merged ASAP, Thanks!

@AkashKanni
Copy link
Contributor Author

/retest

var _ = Describe("List resources",
labels.Day2,
labels.FeatureMachinepool,
labels.NonHCPCluster,
Copy link
Contributor

Choose a reason for hiding this comment

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

@AkashKanni Please answer to this ?

@@ -18,7 +18,7 @@ type ClusterService interface {
DescribeCluster(clusterID string) (bytes.Buffer, error)
ReflectClusterDescription(result bytes.Buffer) (*ClusterDescription, error)
DescribeClusterAndReflect(clusterID string) (*ClusterDescription, error)
List() (bytes.Buffer, error)
ListCluster(flags ...string) (ListCluster, bytes.Buffer, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update following this ?

Copy link
Contributor

openshift-ci bot commented Sep 27, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AkashKanni
Once this PR has been reviewed and has the lgtm label, please assign ciaranroche for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@radtriste
Copy link
Contributor

@AkashKanni Looks like there some merge conflicts. Could you rebase please ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants