hotfix: return error instead of log at `FromMapAndOption` #5381

chansuke · 2023-10-12T13:33:11Z

k8s-ci-robot · 2023-10-12T13:33:15Z

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

k8s-ci-robot · 2023-10-12T13:33:21Z

Welcome @chansuke!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-10-12T13:33:22Z

Hi @chansuke. 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/test-infra repository.

api/internal/plugins/utils/utils_test.go

DiptoChakrabarty · 2023-10-13T17:17:44Z

also seems like the tests are failing can you go ahead and fix them

koba1t

I think we should avoid the fatal function.
But we are currently using log.Fatal(), and we can fix it with the next PR.
https://github.com/kubernetes-sigs/kustomize/pull/5381/files#diff-9041e805f414eb549e8ebcf4f2b82c1f40403e3ebe86a1f67f9c76718bd91797L65

I think this PR is moving forward to the first step and is useful for kustomize.

@chansuke
Thanks for your contribution!
I added any comments, Could you check it?

koba1t · 2023-10-14T16:35:03Z

api/internal/plugins/utils/utils_test.go

-		err = expected.Append(makeConfigMapOptions(rf, name, c.behavior, !c.needsHash))
+		config, err := makeConfigMapOptions(rf, name, c.behavior, !c.needsHash)
+		if err != nil {
+			t.Errorf("unexpected error: %v", err)


I think the unexpected error message is confusing.
Could you add any meaningful message?

koba1t · 2023-10-14T16:35:19Z

api/resmap/reswrangler_test.go

-		}))
+		})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)


koba1t · 2023-10-14T16:35:31Z

api/resmap/reswrangler_test.go

-		}))
+		})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)


koba1t · 2023-10-14T16:35:41Z

api/resmap/reswrangler_test.go

-	assert.NoError(t, w.AbsorbAll(makeMap2(types.BehaviorMerge)))
+		})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)


koba1t · 2023-10-14T16:38:04Z

api/resmap/reswrangler_test.go

+	err = w.AbsorbAll(w2)
 	assert.Error(t, err)


If you can, Could you summarize it in one line?

koba1t · 2023-10-14T16:41:15Z

api/resource/factory.go

-	return rf.FromMapAndOption(m, nil)
+	res, err := rf.FromMapAndOption(m, nil)
+	if err != nil {
+		log.Fatal("Option must not be null")


It looks like this line abandoned a non-nil error.
Could you make a change to print the substance of the returned error?

koba1t · 2023-10-14T16:41:22Z

api/resource/factory.go

-	r := rf.FromMapAndOption(m, nil)
+	r, err := rf.FromMapAndOption(m, nil)
+	if err != nil {
+		log.Fatal("Option must not be null")


chansuke · 2023-10-15T04:15:10Z

@koba1t I appreciate your feedback and review! I will go fix them.

chansuke · 2023-10-15T05:29:14Z

@koba1t I've addressed your comments and made the necessary changes. Please take a look 🙏
And I'd like to work on this in the next PR if it's possible 🙇

I think we should avoid the fatal function.
But we are currently using log.Fatal(), and we can fix it with the next PR.

natasha41575 · 2023-10-16T17:07:41Z

/ok-to-test

koba1t · 2023-10-17T19:05:24Z

/label tide/merge-method-squash

koba1t · 2023-10-17T19:29:05Z

@chansuke

Thanks for the fix!
Looks good for me!

/lgtm

@natasha41575
Would you like to review this PR, or do you have any opinions?
In the meantime, I look forward to your reply.

natasha41575 · 2023-10-19T14:47:34Z

@koba1t Thanks! I'll take a look on friday

natasha41575 · 2023-10-20T15:15:31Z

api/resource/factory.go

-	return rf.FromMapAndOption(m, nil)
+	res, err := rf.FromMapAndOption(m, nil)
+	if err != nil {
+		log.Fatalf("failed to create resource from map: %v", err)


I think the idea of the TODO was to return the error all the way to the top, e.g. completely avoid the use of log.Fatalf for this. That would mean FromMap and FromMapWithNamespaceAndName should the error, instead of doing log.Fatalf here, and likewise all the way up.

Looks like you and @koba1t already had a discussion about potentially doing that in a follow-up PR. I think that's fine to do in a followup, but in that case could we keep the TODO here?

if err != nil { // TODO: return err instead of log. log.Fatalf("failed to create resource from map: %v", err) }

@natasha41575 Thank you for the review. Yes, I will put the TODO comment again

natasha41575 · 2023-10-20T15:15:37Z

api/resource/factory.go

-	r := rf.FromMapAndOption(m, nil)
+	r, err := rf.FromMapAndOption(m, nil)
+	if err != nil {
+		log.Fatalf("failed to create resource from map: %v", err)


chansuke · 2023-10-22T10:47:59Z

@natasha41575 Updated the code. Please take a look.

natasha41575

/lgtm
/approve

k8s-ci-robot · 2023-10-27T20:33:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chansuke, natasha41575

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~~ [natasha41575]

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

LGTM was given above

…-sigs#5381) * hotfix: return error instead of log at `FromMapAndOption` * chore: show error message * hotfix: use correct function * hotix: use `t.Helper()` and fix `t *testing.T order * hotfix: wrapt the error of `FromMapAndOption` * hotfix: meaningful message for an error * hotfix: summarize in one line * hotfix: fix the abandoned error and show meaningful message * hotfix: start with helper function * Keep TODO comment

chansuke added 2 commits October 12, 2023 22:19

hotfix: return error instead of log at FromMapAndOption

7f3334c

chore: show error message

5eb100c

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 12, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 12, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 12, 2023

k8s-ci-robot requested review from koba1t and yuwenma October 12, 2023 13:33

DiptoChakrabarty reviewed Oct 13, 2023

View reviewed changes

api/internal/plugins/utils/utils_test.go Outdated Show resolved Hide resolved

chansuke added 3 commits October 14, 2023 13:14

hotfix: use correct function

1849ddf

hotix: use t.Helper() and fix `t *testing.T order

c8172de

hotfix: wrapt the error of FromMapAndOption

18ba44d

chansuke requested a review from DiptoChakrabarty October 14, 2023 08:22

koba1t previously requested changes Oct 14, 2023

View reviewed changes

chansuke added 3 commits October 15, 2023 13:51

hotfix: meaningful message for an error

6752985

hotfix: summarize in one line

da28bfa

hotfix: fix the abandoned error and show meaningful message

6cd6816

chansuke force-pushed the return_err_insteadof_log branch from 171d984 to 6cd6816 Compare October 15, 2023 05:20

chansuke requested a review from koba1t October 15, 2023 05:29

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 Oct 16, 2023

hotfix: start with helper function

54b6056

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 17, 2023

k8s-ci-robot assigned koba1t Oct 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2023

natasha41575 suggested changes Oct 20, 2023

View reviewed changes

Keep TODO comment

aa7984c

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2023

chansuke requested a review from natasha41575 October 21, 2023 06:26

natasha41575 reviewed Oct 27, 2023

View reviewed changes

k8s-ci-robot assigned natasha41575 Oct 27, 2023

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. labels Oct 27, 2023

natasha41575 approved these changes Oct 27, 2023

View reviewed changes

k8s-ci-robot merged commit e002b49 into kubernetes-sigs:master Oct 27, 2023
9 checks passed

chansuke mentioned this pull request Mar 27, 2024

fix: return error instead of log.Fatalf() #5625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hotfix: return error instead of log at `FromMapAndOption` #5381

hotfix: return error instead of log at `FromMapAndOption` #5381

chansuke commented Oct 12, 2023 •

edited

k8s-ci-robot commented Oct 12, 2023

k8s-ci-robot commented Oct 12, 2023

k8s-ci-robot commented Oct 12, 2023

DiptoChakrabarty commented Oct 13, 2023

koba1t left a comment

koba1t Oct 14, 2023

koba1t Oct 14, 2023

koba1t Oct 14, 2023

koba1t Oct 14, 2023

koba1t Oct 14, 2023

koba1t Oct 14, 2023

koba1t Oct 14, 2023

chansuke commented Oct 15, 2023

chansuke commented Oct 15, 2023

natasha41575 commented Oct 16, 2023

koba1t commented Oct 17, 2023

koba1t commented Oct 17, 2023

natasha41575 commented Oct 19, 2023

natasha41575 Oct 20, 2023 •

edited

chansuke Oct 21, 2023

natasha41575 Oct 20, 2023

chansuke commented Oct 22, 2023

natasha41575 left a comment

k8s-ci-robot commented Oct 27, 2023

hotfix: return error instead of log at FromMapAndOption #5381

hotfix: return error instead of log at FromMapAndOption #5381

Conversation

chansuke commented Oct 12, 2023 • edited

k8s-ci-robot commented Oct 12, 2023

k8s-ci-robot commented Oct 12, 2023

k8s-ci-robot commented Oct 12, 2023

DiptoChakrabarty commented Oct 13, 2023

koba1t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chansuke commented Oct 15, 2023

chansuke commented Oct 15, 2023

natasha41575 commented Oct 16, 2023

koba1t commented Oct 17, 2023

koba1t commented Oct 17, 2023

natasha41575 commented Oct 19, 2023

natasha41575 Oct 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chansuke commented Oct 22, 2023

natasha41575 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 27, 2023

hotfix: return error instead of log at `FromMapAndOption` #5381

hotfix: return error instead of log at `FromMapAndOption` #5381

chansuke commented Oct 12, 2023 •

edited

natasha41575 Oct 20, 2023 •

edited