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

UT for ReconcileCOSInstance functionality #2035

Merged
merged 1 commit into from
Nov 13, 2024
Merged

UT for ReconcileCOSInstance functionality #2035

merged 1 commit into from
Nov 13, 2024

Conversation

carmal891
Copy link
Contributor

@carmal891 carmal891 commented Nov 5, 2024 •
edited
Loading

What this PR does / why we need it:

UT for ReconcileCOSInstance functionality

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # Part of #1818

Special notes for your reviewer:

/area provider/ibmcloud

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

@k8s-ci-robot k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Nov 5, 2024
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @carmal891. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 5, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 5, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name Link
🔨 Latest commit 0ab00b7
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/673440f99082980008848ae5
😎 Deploy Preview https://deploy-preview-2035--kubernetes-sigs-cluster-api-ibmcloud.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@carmal891
Copy link
Contributor Author

Discussed and confirmed with @Karthik-K-N to omit portions that cover control flow over checkCOSBucket and createCOSBucket within the TestReconcileCOSInstance function. Instead it can be addressed as part of #2034

@Amulyam24
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 6, 2024
Karthik-K-N
Karthik-K-N suggested changes Nov 7, 2024
View reviewed changes
cloud/scope/powervs_cluster_test.go Outdated
},
}

mockResourceController.EXPECT().GetInstanceByName(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("Fetch failed"))
Copy link
Contributor

Choose a reason for hiding this comment

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

In general we follow lowercase for error statements, Though it might not matter here better to follow and also elaborate a bit like

error getting instance by name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sry missed out. Done

cloud/scope/powervs_cluster_test.go Outdated

mockResourceController.EXPECT().GetInstanceByName(gomock.Any(), gomock.Any(), gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{
Name: ptr.To("test-cos-resource-name"),
State: ptr.To("active"),
Copy link
Contributor

Choose a reason for hiding this comment

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

not a blocking one but we can use like this as well ,( Here and at all other relevant places).

ptr.To(string(infrav1beta2.ServiceInstanceStateActive))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cloud/scope/powervs_cluster_test.go
},
}

mockResourceController.EXPECT().GetInstanceByName(gomock.Any(), gomock.Any(), gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also handle the case where cos instance does not exist and GetInstanceByName returns nil? so we endup creating cos instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cloud/scope/powervs_cluster_test.go
}, nil)

err := clusterScope.ReconcileCOSInstance()
g.Expect(err).ToNot(BeNil())
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible if we can check status as well https://github.com/Karthik-K-N/cluster-api-provider-ibmcloud/blob/987191d98e364d4bbfc39423d433b791dbb6b102/cloud/scope/powervs_cluster.go#L2171, that will be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cloud/scope/powervs_cluster_test.go Outdated
g.Expect(err).ToNot(BeNil())
})

t.Run("When successfully creates resource instance", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Run("When successfully creates resource instance", func(t *testing.T) {
t.Run("When create resource instance is successfull", func(t *testing.T) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@carmal891 carmal891 requested a review from Karthik-K-N November 7, 2024 07:23
Karthik-K-N
Karthik-K-N reviewed Nov 7, 2024
View reviewed changes
Copy link
Contributor

@Karthik-K-N Karthik-K-N left a comment

Choose a reason for hiding this comment

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

One qq, Otherwise LGTM, Thanks

cloud/scope/powervs_cluster_test.go Outdated
g.Expect(cosResourceInstance).To(BeNil())
g.Expect(err).To(BeNil())
})
t.Run("When cos service instance state is not active", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

whats the difference between this and next test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

apologies redundant test. Removed

@carmal891 carmal891 requested a review from Karthik-K-N November 7, 2024 08:49
Amulyam24
Amulyam24 reviewed Nov 7, 2024
View reviewed changes
cloud/scope/powervs_cluster_test.go
g.Expect(clusterScope.IBMPowerVSCluster.Status.COSInstance).To(BeNil())
})

t.Run("When COS service instance is not found in IBM Cloud and hence creates COS instance in cloud", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about the case where a COS service instance with a given name exists in cloud and controllerCreated is set to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

cloud/scope/powervs_cluster_test.go Outdated
g.Expect(clusterScope.IBMPowerVSCluster.Status.COSInstance.ControllerCreated).To(Equal(ptr.To(bool(true))))
})

t.Run("When check on whether bucket exist in service instance fails", func(t *testing.T) {
Copy link
Contributor

@Amulyam24 Amulyam24 Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

can you share what scenario this is covering? I'm not quite clear here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This covers the scenario where checkCOSBucket function returns unexpected aws error (aws error apart from s3.ErrCodeNoSuchBucket, "Forbidden" & "NotFound")

Although there is the highlighted cos client issue #2034 we can still add coverage over lines mentioned below. Hence added this test case covering flow over checkCOSBucket

cluster-api-provider-ibmcloud/cloud/scope/powervs_cluster.go

Lines 2221 to 2224 in b4a2ff4

} else if err != nil {
s.Error(err, "failed to check COS bucket")
return err
}

I will update the description to
When checkCOSBucket fails to determine whether bucket exist in cloud due to an unexpected error to make it more meaningful

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, Thanks for clarifying!

cloud/scope/powervs_cluster_test.go Outdated
mockCtrl.Finish()
}

t.Run("When creating cos resource instance fails due to no resource group id", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Run("When creating cos resource instance fails due to no resource group id", func(t *testing.T) {
t.Run("When creating cos resource instance fails due to missing resource group id", func(t *testing.T) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cloud/scope/powervs_cluster_test.go Outdated
testScenarios := []struct {
name string
mockError error
expectedExists bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expectedExists bool
bucketExists bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Amulyam24
Amulyam24 reviewed Nov 7, 2024
View reviewed changes
cloud/scope/powervs_cluster_test.go Outdated
g.Expect(err).ToNot(BeNil())
})

t.Run("When create resource instance is successful", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Run("When create resource instance is successful", func(t *testing.T) {
t.Run("When COS resource instance creation is successful", func(t *testing.T) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Amulyam24
Amulyam24 reviewed Nov 7, 2024
View reviewed changes
cloud/scope/powervs_cluster_test.go Outdated
teardown := func() {
mockCtrl.Finish()
}
t.Run("When checking COS bucket existence", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Run("When checking COS bucket existence", func(t *testing.T) {
t.Run("When checking if COS bucket exists in cloud", func(t *testing.T) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@carmal891 carmal891 requested a review from Amulyam24 November 7, 2024 15:27
Amulyam24
Amulyam24 approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@Amulyam24 Amulyam24 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2024
@carmal891
Copy link
Contributor Author

assign @mkumatag

@Amulyam24
Copy link
Contributor

@carmal891 , please squash the commits

@carmal891 carmal891 force-pushed the UT-ReconcileCOSInstance branch from 76ca1ba to a990e3e Compare November 12, 2024 10:43
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2024
@carmal891
Copy link
Contributor Author

@carmal891 , please squash the commits

Done

Prajyot-Parab
Prajyot-Parab approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@Prajyot-Parab Prajyot-Parab left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 12, 2024
@Prajyot-Parab
Copy link
Contributor

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2024
@Karthik-K-N
Copy link
Contributor

@carmal891 could you please rebase the PR.

@carmal891 carmal891 force-pushed the UT-ReconcileCOSInstance branch from a990e3e to 52de141 Compare November 13, 2024 05:59
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 13, 2024
@carmal891 carmal891 force-pushed the UT-ReconcileCOSInstance branch from 52de141 to 0ab00b7 Compare November 13, 2024 06:02
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 13, 2024
@carmal891
Copy link
Contributor Author

@carmal891 could you please rebase the PR.

Done

@Prajyot-Parab
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2024
Prajyot-Parab
Prajyot-Parab approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@Prajyot-Parab Prajyot-Parab left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Amulyam24, carmal891, Prajyot-Parab

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

@k8s-ci-robot k8s-ci-robot merged commit ea7c08b into kubernetes-sigs:main Nov 13, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Prajyot-Parab Prajyot-Parab Prajyot-Parab approved these changes

@Amulyam24 Amulyam24 Amulyam24 approved these changes

@Karthik-K-N Karthik-K-N Awaiting requested review from Karthik-K-N

Assignees

@Amulyam24 Amulyam24

@Prajyot-Parab Prajyot-Parab

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/ibmcloud Issues or PRs related to ibmcloud provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@carmal891 @k8s-ci-robot @Amulyam24 @Prajyot-Parab @Karthik-K-N