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.

Sign up for GitHub

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

Jump to bottom

e2e test: unified the clients #177

Merged
merged 2 commits into from
May 19, 2022
Merged

e2e test: unified the clients #177

merged 2 commits into from
May 19, 2022

Conversation

riya-singhal31
Copy link
Contributor

replaced all clients with a controller runtime client.

Signed-off-by: riya-singhal31 [email protected]

@riya-singhal31 riya-singhal31 changed the title test: unified the clients e2e test: unified the clients Apr 27, 2022
nbalacha
nbalacha suggested changes May 4, 2022
View reviewed changes
pkg/deploymanager/subscription.go Outdated

if !isReady {
lastReason = "waiting on lvm catalog source pod to reach ready state"
if existingCatalogSource.Status.GRPCConnectionState.LastObservedState != "READY" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the cataogSource status equivalent to the pod status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

pkg/deploymanager/subscription.go Outdated
@@ -158,14 +139,15 @@ func (t *DeployManager) WaitForLVMOperator() error {

err := utilwait.PollImmediate(interval, timeout, func() (done bool, err error) {
for _, name := range deployments {
deployment, err := t.k8sClient.AppsV1().Deployments(InstallNamespace).Get(context.TODO(), name, metav1.GetOptions{})
existingdeployment := &appsv1.Deployment{}
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable name need not be changed. It was checking for existing deployments earlier as well.

pkg/deploymanager/subscription.go Outdated
if err != nil {
return err
}
if len(operatorGroups.Items) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

The error checks are required.

pkg/deploymanager/subscription.go Outdated
if err != nil && !errors.IsNotFound(err) {
return err
}

}

for _, catalogSource := range co.catalogSources {
err := t.olmClient.OperatorsV1alpha1().CatalogSources(catalogSource.Namespace).Delete(context.TODO(), catalogSource.Name, metav1.DeleteOptions{})
catalogSource := catalogSource
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a diff name for the inner loop var.

pkg/deploymanager/config.go
return t.lvmClient
}

func getKubeconfig(kubeconfig string) (*rest.Config, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can leave this function for better readibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

means to use the separate function for geting kubeconfig?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated that.

pkg/deploymanager/config.go Outdated
lvmClient: lvmClient,
parameterCodec: parameterCodec,
crClient: crClient,
Ctx: context.TODO(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the ctx from the deploymanager. Lets stick to context.TODO for now.

pkg/deploymanager/subscription.go Outdated
})
err := utilwait.PollImmediate(interval, timeout, func() (done bool, err error) {
existingCatalogSource := &v1alpha1.CatalogSource{}
err = t.crClient.Get(t.Ctx, client.ObjectKeyFromObject(catalogsource), existingCatalogSource)
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 just use the catalogSource name?

@riya-singhal31 riya-singhal31 force-pushed the unifying-client branch 3 times, most recently from c77dc25 to 261d4f5 Compare May 5, 2022 07:42
@riya-singhal31
Copy link
Contributor Author

Updated the PR with all the changes.

nbalacha
nbalacha reviewed May 5, 2022
View reviewed changes
pkg/deploymanager/subscription.go Outdated
@@ -97,7 +99,7 @@ func (t *DeployManager) generateClusterObjects(lvmCatalogImage string, subscript
}

// Waiting for LVM catalog source.
func (t *DeployManager) waitForLVMCatalogSource() error {
func (t *DeployManager) waitForLVMCatalogSource(catalogsource *v1alpha1.CatalogSource) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is enough to just pass the name of the catalogsource,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Name is also not required now as we are following the previous method itself.

nbalacha
nbalacha reviewed May 12, 2022
View reviewed changes
pkg/deploymanager/subscription.go Outdated
if err != nil {
return err
}
// Wait for catalog source before posting subscription
Copy link
Contributor

Choose a reason for hiding this comment

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

Why has this been moved inside the for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was passing catalog source as an argument to the waitForLVMCatalogSource call, so thats why did that. But now its of no use.
Shall I move it outside?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes,please move it outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2022
openshift-ci bot and others added 2 commits May 13, 2022 17:57
@openshift-ci @riya-singhal31
Merge pull request openshift#190 from riya-singhal31/snapshot-e2e
2614358 
fix: added volume snapshot api to e2e client scheme
@riya-singhal31
test: unified the clients
11dff04 
replaced all clients with a controller runtime client.

Signed-off-by: riya-singhal31 <[email protected]>
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 13, 2022

@riya-singhal31: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/lvm-operator-bundle-e2e-aws 11dff04 link false /test lvm-operator-bundle-e2e-aws

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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nbalacha, riya-singhal31

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2022
@openshift-merge-robot openshift-merge-robot merged commit e6a44fe into openshift:main May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbalacha nbalacha nbalacha approved these changes

Assignees

@nbalacha nbalacha

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@riya-singhal31 @nbalacha @openshift-merge-robot