WIP: 🌱 added tests for loadTLSConfigFromEnv in factory_test.go #1589

danish9039 · 2024-03-04T20:21:04Z

This commit introduces a set of comprehensive tests for the in theloadingTLSconfiguration in"factory_test.go" file of the ironic package. The tests cover various scenarios and edge cases related to loading configuration from environment variables, handling endpoints, and loading TLSconfiguration.

What this PR does / why we need it:

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

metal3-io-bot · 2024-03-04T20:21:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kashifest 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:

OWNERS

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

metal3-io-bot · 2024-03-04T20:21:15Z

Hi @danish9039. Thanks for your PR.

I'm waiting for a metal3-io 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/test-infra repository.

tuminoid · 2024-03-05T07:14:10Z

Please sign-off your commit to fulfill DCO requirement.

/ok-to-test

lentzi90 · 2024-03-05T09:07:23Z

Thanks for the PR!
You will need to fix the test failures. Try running make lint to see the issues in your environment also.

This commit introduces a set of comprehensive tests for the factory functions in the "factory_test.go" file of the ironic package. The tests cover various scenarios and edge cases related to loading configuration from environment variables, handling endpoints, and loading TLS configuration. Signed-off-by: danish siddiqui <[email protected]>

Signed-off-by: danish siddiqui <[email protected]>

lentzi90 · 2024-03-06T06:23:09Z

Could you update the description so it says "Part of #1574" instead of "Fixes", since the issue has multiple parts?

danish9039 · 2024-03-06T06:30:06Z

Could you update the description so it says "Part of #1574" instead of "Fixes", since the issue has multiple parts?

sure

lentzi90 · 2024-03-06T06:28:31Z

pkg/provisioner/ironic/factory_test.go

@@ -181,35 +181,78 @@ func TestLoadConfigFromEnv(t *testing.T) {
 	}
 }

-func TestLoadEndpointsFromEnv(t *testing.T) {


Do I understand correctly that you removed this test? Why?
I think we should keep it and add a separate test for the TLS config.

lentzi90 · 2024-03-06T06:32:42Z

pkg/provisioner/ironic/factory_test.go

+	os.Setenv("IRONIC_CACERT_FILE", "/path/to/ca.crt")
+	os.Setenv("IRONIC_CLIENT_CERT_FILE", "/path/to/client.crt")
+	os.Setenv("IRONIC_CLIENT_PRIVATE_KEY_FILE", "/path/to/client.key")
+	os.Setenv("IRONIC_INSECURE", "false")
+	os.Setenv("IRONIC_SKIP_CLIENT_SAN_VERIFY", "true")
+	defer func() {
+		os.Unsetenv("IRONIC_CACERT_FILE")
+		os.Unsetenv("IRONIC_CLIENT_CERT_FILE")
+		os.Unsetenv("IRONIC_CLIENT_PRIVATE_KEY_FILE")
+		os.Unsetenv("IRONIC_INSECURE")
+		os.Unsetenv("IRONIC_SKIP_CLIENT_SAN_VERIFY")
+	}()


If you check the other tests in this file, you will see that they make use of a struct EnvFixture. There are helpful functions for setting and unsetting the environment from this struct. Put all of this together and you get a nice way of looping through test cases described by structs. Could you try to mimic this?

Signed-off-by: danish siddiqui <[email protected]>

Rozzii · 2024-06-28T10:34:42Z

/cc @mquhuy @kashifest

tuminoid · 2024-07-01T10:08:19Z

/retitle WIP: 🌱 added tests for loadTLSConfigFromEnv in factory_test.go

There are open questions from @lentzi90 that require changes to the PR, marking as WIP.

metal3-io-bot · 2024-10-01T14:25:52Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

Rozzii · 2024-10-04T05:46:07Z

IMO we should leave the "stale" on this until the questions from @lentzi90 are answered.
Are you still willing to work on this @danish9039 ?

metal3-io-bot · 2024-11-03T05:48:16Z

Stale issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle stale.

/close

metal3-io-bot · 2024-11-03T05:48:20Z

@metal3-io-bot: Closed this PR.

In response to this:

Stale issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle stale.

/close

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.

metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 4, 2024

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 4, 2024

metal3-io-bot 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 Mar 5, 2024

lentzi90 mentioned this pull request Mar 5, 2024

Unit test coverage improvement in BMO #1574

Closed

7 tasks

metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 5, 2024

danish9039 and others added 3 commits March 6, 2024 01:04

updated TestLoadTLSConfigFromEnvin factory_test.go

cd9d57b

Signed-off-by: danish siddiqui <[email protected]>

fix lint errors

aef07f5

Signed-off-by: danish siddiqui <[email protected]>

danish9039 force-pushed the patch-1 branch from 4097485 to aef07f5 Compare March 5, 2024 19:35

lentzi90 reviewed Mar 6, 2024

View reviewed changes

fixed tests

8578b40

Signed-off-by: danish siddiqui <[email protected]>

metal3-io-bot requested review from kashifest and mquhuy June 28, 2024 10:34

Rozzii added this to the BMO - v0.7.0 milestone Jun 28, 2024

metal3-io-bot changed the title ~~🌱 added tests for loadTLSConfigFromEnv in factory_test.go~~ WIP: 🌱 added tests for loadTLSConfigFromEnv in factory_test.go Jul 1, 2024

metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 1, 2024

Rozzii modified the milestones: BMO - v0.6.2, BMO - v0.7.0 Jul 3, 2024

metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 1, 2024

metal3-io-bot closed this Nov 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: 🌱 added tests for loadTLSConfigFromEnv in factory_test.go #1589

WIP: 🌱 added tests for loadTLSConfigFromEnv in factory_test.go #1589

danish9039 commented Mar 4, 2024 •

edited

Loading

metal3-io-bot commented Mar 4, 2024

metal3-io-bot commented Mar 4, 2024

tuminoid commented Mar 5, 2024

lentzi90 commented Mar 5, 2024

lentzi90 commented Mar 6, 2024

danish9039 commented Mar 6, 2024

lentzi90 Mar 6, 2024

lentzi90 Mar 6, 2024

Rozzii commented Jun 28, 2024

tuminoid commented Jul 1, 2024

metal3-io-bot commented Oct 1, 2024

Rozzii commented Oct 4, 2024

metal3-io-bot commented Nov 3, 2024

metal3-io-bot commented Nov 3, 2024

WIP: 🌱 added tests for loadTLSConfigFromEnv in factory_test.go #1589

WIP: 🌱 added tests for loadTLSConfigFromEnv in factory_test.go #1589

Conversation

danish9039 commented Mar 4, 2024 • edited Loading

metal3-io-bot commented Mar 4, 2024

metal3-io-bot commented Mar 4, 2024

tuminoid commented Mar 5, 2024

lentzi90 commented Mar 5, 2024

lentzi90 commented Mar 6, 2024

danish9039 commented Mar 6, 2024

lentzi90 Mar 6, 2024

Choose a reason for hiding this comment

lentzi90 Mar 6, 2024

Choose a reason for hiding this comment

Rozzii commented Jun 28, 2024

tuminoid commented Jul 1, 2024

metal3-io-bot commented Oct 1, 2024

Rozzii commented Oct 4, 2024

metal3-io-bot commented Nov 3, 2024

metal3-io-bot commented Nov 3, 2024

danish9039 commented Mar 4, 2024 •

edited

Loading