kubeadm: Replace the yaml in the log/comments with a generic term. #130345

HirazawaUi · 2025-02-21T14:46:41Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Currently, kubeadm supports both yaml and json configuration file types, but in our log output, we only explicitly mention the yaml format.
We haven't printed warning logs for unknown GVKs in all the documentMapToXXX functions. To ensure consistent behavior across the documentMapToXXX functions and to provide sufficient warnings to users, we should print warning logs.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#3163 kubernetes/kubeadm#3162

Special notes for your reviewer:

To make the functions appear more consistent, I also renamed the SplitYAMLDocuments function to SplitYAMLOrJSONDocuments. If this is an unnecessary change, I can roll it back.

Does this PR introduce a user-facing change?

kubeadm: Use generic terminology in logs instead of direct mentions of yaml/json.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2025-02-21T14:46:52Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

carlory · 2025-02-24T02:36:53Z

/release-note-none
/lgtm

k8s-ci-robot · 2025-02-24T02:36:59Z

LGTM label has been added.

Git tree hash: 4de94b209ce33e899d62167ce8ea44456d8afa39

neolit123

thanks for the pr @HirazawaUi

cmd/kubeadm/app/cmd/config_test.go

cmd/kubeadm/app/util/config/initconfiguration.go

cmd/kubeadm/app/util/config/joinconfiguration.go

cmd/kubeadm/app/util/config/upgradeconfiguration.go

cmd/kubeadm/app/util/config/upgradeconfiguration_test.go

cmd/kubeadm/app/util/marshal.go

neolit123 · 2025-02-24T09:13:03Z

cmd/kubeadm/app/util/config/upgradeconfiguration_test.go

+		for _, format := range formats {
+			t.Run(fmt.Sprintf("%s_%s", tt.name, format.name), func(t *testing.T) {


since you are adding multi format testing for init and upgrade config, should you also add it to reset and join config?

neolit123 · 2025-02-24T09:13:58Z

Does this PR introduce a user-facing change?

needs a simple release note explaining the user facing aspect of the change.

cmd/kubeadm/app/util/config/common.go

cmd/kubeadm/app/util/config/joinconfiguration.go

neolit123 · 2025-02-25T14:34:54Z

cmd/kubeadm/app/util/config/joinconfiguration_test.go

 				}

-				if obj == nil {
-					t.Error("Unexpected nil return value")
+				got, _ := LoadJoinConfigurationFromFile(tt.cfgPath, options)


why not use bytestojoinconfiguration instead of writing a file and reading it
we should make this consistent across xconfiguration_tests.go.

cmd/kubeadm/app/util/config/resetconfiguration.go

neolit123 · 2025-02-25T14:37:15Z

cmd/kubeadm/app/util/config/resetconfiguration_test.go

 				}

-				if obj == nil {
-					t.Error("Unexpected nil return value")
+				got, _ := LoadResetConfigurationFromFile(tt.cfgPath, options)


again we should use bytestoresetconfiguration here instead of writing and loading

the load* functions can have another simple test, maybe

cmd/kubeadm/app/util/config/upgradeconfiguration.go

neolit123 · 2025-02-26T16:02:07Z

running some e2e
https://github.com/neolit123/kubeadm-test/actions/runs/13548136535
edit; it's passing

neolit123

there are still some minor consistency problems, but the refactor is turning out nicely.
thanks

cmd/kubeadm/app/util/config/joinconfiguration_test.go

cmd/kubeadm/app/util/config/resetconfiguration_test.go

neolit123 · 2025-02-26T16:10:17Z

cmd/kubeadm/app/util/config/upgradeconfiguration_test.go

-				t.Errorf("LoadOrDefaultUpgradeConfiguration returned unexpected diff (-want,+got):\n%s", diff)
-			}
-		})
+				got, _ := LoadOrDefaultUpgradeConfiguration(tt.cfgPath, tt.cfg, options)


we should remove the TestLoadUpgradeConfigurationFromFile function if other xconfiguration_test files no longer test the load functions and move the relevant test cases to the test function that tests the BytesTo* function

or just add a very simple TestLoadOrDefaultXConfiguration test everywhere that just makes sure that the function doesn't fail for a simple config.

I added TestLoadXFromFile tests for reset and join, but could not add one for init since ComponentConfigs cannot be exported.

not sure i understand, can you please provide an example?

For example:

The kubeletConfig struct in the componentconfigs directory is unexported (non-public).
ref:

kubernetes/cmd/kubeadm/app/componentconfigs/kubelet.go

Lines 84 to 87 in 8cca6d9

type kubeletConfig struct {

configBase

config kubeletconfig.KubeletConfiguration

}

When executing the BytesToInitConfiguration function, we will assign
default values to InitConfiguration.ComponentConfigs["kubelet.config.k8s.io"].kubeletConfig.

However, since kubeletConfig is unexported, we cannot reference it in the util/config directory, making comparisons impossible.

wouldn't this work?

cmp.Diff(got, tt.want, cmpopts.IgnoreFields{kubeadmapi.InitConfiguration{}, "ComponentConfigs"})

also i checked the latest contents of the latest pr, here is a more detailed explanation, what i meant earlier:

for each xconfiguration_test.go file, we should have:

a detailed test for TestBytesToXConfiguration with various test cases

a TestLoadXConfigurationFromFile with just a single case testing that a file loads, that's all

cmp.Diff(got, tt.want, cmpopts.IgnoreFields{kubeadmapi.InitConfiguration{}, "ComponentConfigs"})

Yes, this will work perfectly.

a TestLoadXConfigurationFromFile with just a single case testing that a file loads, that's all

I think I understand your point. However, replicating the tests for TestLoadXConfigurationFromFile is quite convenient for me, so I provided them with the same test cases as TestLoadUpgradeConfigurationFromFile (my initial thought was that more test cases are always better than simple ones, especially when they are straightforward to copy/implement). Would this be considered redundant? If there are other considerations and we only need a simple test case, I can revert these unit tests.

yes, its redundant. for the TestLoadXConfigurationFromFile, we just need a simple load test without content comparison.

All right. I will add the unit tests for BytesToInitConfiguration tomorrow and revert some of the test cases from the latest commit.

Changes have been made based on comments.

HirazawaUi · 2025-03-01T04:02:46Z

/retest

neolit123

you could organize the commits better. remove all _test.go changes from commit 1 as it's supposed to only change output and move them to the last commits about test consistency. currently commit 1 makes changes to tests and later the last commit also changes the same tests.

also for all TestLoadXConfigurationFromFile functions make it use the two test cases:

missing file
valid config

neolit123 · 2025-03-03T10:59:35Z

cmd/kubeadm/app/util/config/upgradeconfiguration_test.go

 			if (err != nil) != tc.expectedError {
 				t.Fatalf("failed DocMapToUpgradeConfiguration:\n\texpected error: %t\n\t  actual error: %v", tc.expectedError, err)
 			}
 			if err == nil {
 				if diff := cmp.Diff(*cfg, tc.expectedCfg, cmpopts.IgnoreFields(kubeadmapi.UpgradeConfiguration{}, "Timeouts")); diff != "" {
-					t.Fatalf("DocMapToUpgradeConfiguration returned unexpected diff (-want,+got):\n%s", diff)
+					t.Fatalf("BytesToUpgradeConfiguration returned unexpected diff (-want,+got):\n%s", diff)


move this change from commit add BytesToXConfiguration function to the last commit with tests.

Checking and fixing problems caused by rebase, just a moment.

I still put the changes to this unit test in the fourth commit because we removed the DocMapToUpgradeConfiguration function in this commit. If we don’t modify this unit test, it will fail in the fourth commit.

isn't it possible to move the delete of DocMapToUpgradeConfiguration also in the last commit?
the delete seems like a unit tests refactor too.

Makes sense and has been changed as suggested.

HirazawaUi · 2025-03-03T11:19:21Z

you could organize the commits better. remove all _test.go changes from commit 1 as it's supposed to only change output and move them to the last commits about test consistency. currently commit 1 makes changes to tests and later the last commit also changes the same tests.

also for all TestLoadXConfigurationFromFile functions make it use the two test cases:

missing file

valid config

The content of the historical commits has been modified according to this suggestion.

neolit123 · 2025-03-03T14:04:55Z

/lgtm
/approve
thanks

/hold
let's remove the hold once ci passes here and in the external repo:
https://github.com/neolit123/kubeadm-test/actions/runs/13632680478/job/38103832447
https://github.com/neolit123/kubeadm-test/actions/runs/13632722131/job/38103965538

k8s-ci-robot · 2025-03-03T14:05:02Z

LGTM label has been added.

Git tree hash: 636194a98151cf78f663a3457a37c15406e361f3

k8s-ci-robot · 2025-03-03T14:05:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HirazawaUi, neolit123

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:

~~cmd/kubeadm/OWNERS~~ [neolit123]

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

HirazawaUi · 2025-03-03T14:08:32Z

Thank you for your patient review!

neolit123 · 2025-03-03T14:10:01Z

this also fixes

Clarify configuration file handling behavior during loading kubeadm#3163

iiuc

k8s-ci-robot requested review from carlory and neolit123 February 21, 2025 14:47

HirazawaUi force-pushed the kubeadm-yaml-json branch from 8a02227 to fb63528 Compare February 21, 2025 14:57

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 24, 2025

k8s-ci-robot assigned carlory Feb 24, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2025

neolit123 reviewed Feb 24, 2025

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 24, 2025

HirazawaUi force-pushed the kubeadm-yaml-json branch from fb63528 to 43ab8e3 Compare February 24, 2025 12:47

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2025

HirazawaUi force-pushed the kubeadm-yaml-json branch from 43ab8e3 to bf89648 Compare February 24, 2025 12:53

neolit123 reviewed Feb 25, 2025

View reviewed changes

neolit123 reviewed Feb 26, 2025

View reviewed changes

HirazawaUi force-pushed the kubeadm-yaml-json branch 6 times, most recently from cb8b9ba to 3bc4963 Compare February 28, 2025 14:46

neolit123 reviewed Mar 3, 2025

View reviewed changes

HirazawaUi added 3 commits March 3, 2025 18:35

distinguish between YAML and JSON file formats during log output

6a59c98

Add warning logs for uninteresting kind

2d8d972

rename SplitYAMLDocuments function

77647cd

HirazawaUi force-pushed the kubeadm-yaml-json branch 2 times, most recently from 0335aea to a2a6a5e Compare March 3, 2025 10:59

neolit123 reviewed Mar 3, 2025

View reviewed changes

HirazawaUi force-pushed the kubeadm-yaml-json branch from a2a6a5e to cd0a29e Compare March 3, 2025 11:09

HirazawaUi added 2 commits March 3, 2025 21:08

add BytesToXConfiguration function

a94403e

Adding tests for consistency

9a84379

HirazawaUi force-pushed the kubeadm-yaml-json branch from cd0a29e to 9a84379 Compare March 3, 2025 13:10

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2025

k8s-ci-robot assigned neolit123 Mar 3, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Replace the yaml in the log/comments with a generic term. #130345

kubeadm: Replace the yaml in the log/comments with a generic term. #130345

HirazawaUi commented Feb 21, 2025 •

edited

Loading

k8s-ci-robot commented Feb 21, 2025

carlory commented Feb 24, 2025

k8s-ci-robot commented Feb 24, 2025

neolit123 left a comment

neolit123 Feb 24, 2025

HirazawaUi Feb 24, 2025

neolit123 commented Feb 24, 2025

neolit123 Feb 25, 2025

neolit123 Feb 25, 2025

neolit123 commented Feb 26, 2025 •

edited

Loading

neolit123 left a comment

neolit123 Feb 26, 2025 •

edited

Loading

HirazawaUi Feb 27, 2025

neolit123 Feb 27, 2025

HirazawaUi Feb 27, 2025

neolit123 Feb 27, 2025 •

edited

Loading

HirazawaUi Feb 27, 2025

neolit123 Feb 27, 2025 •

edited

Loading

HirazawaUi Feb 27, 2025

HirazawaUi Feb 28, 2025

HirazawaUi commented Mar 1, 2025

neolit123 left a comment •

edited

Loading

neolit123 Mar 3, 2025

HirazawaUi Mar 3, 2025

HirazawaUi Mar 3, 2025

neolit123 Mar 3, 2025 •

edited

Loading

HirazawaUi Mar 3, 2025

HirazawaUi commented Mar 3, 2025

neolit123 commented Mar 3, 2025 •

edited

Loading

k8s-ci-robot commented Mar 3, 2025

k8s-ci-robot commented Mar 3, 2025

HirazawaUi commented Mar 3, 2025

neolit123 commented Mar 3, 2025

		for _, format := range formats {
		t.Run(fmt.Sprintf("%s_%s", tt.name, format.name), func(t *testing.T) {

	type kubeletConfig struct {
	configBase
	config kubeletconfig.KubeletConfiguration
	}

kubeadm: Replace the yaml in the log/comments with a generic term. #130345

Are you sure you want to change the base?

kubeadm: Replace the yaml in the log/comments with a generic term. #130345

Conversation

HirazawaUi commented Feb 21, 2025 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Feb 21, 2025

carlory commented Feb 24, 2025

k8s-ci-robot commented Feb 24, 2025

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Feb 26, 2025 • edited Loading

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 Feb 26, 2025 • edited Loading

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

neolit123 Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HirazawaUi commented Mar 1, 2025

neolit123 left a comment • edited Loading

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

neolit123 Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HirazawaUi commented Mar 3, 2025

neolit123 commented Mar 3, 2025 • edited Loading

k8s-ci-robot commented Mar 3, 2025

k8s-ci-robot commented Mar 3, 2025

HirazawaUi commented Mar 3, 2025

neolit123 commented Mar 3, 2025

HirazawaUi commented Feb 21, 2025 •

edited

Loading

neolit123 commented Feb 26, 2025 •

edited

Loading

neolit123 Feb 26, 2025 •

edited

Loading

neolit123 Feb 27, 2025 •

edited

Loading

neolit123 Feb 27, 2025 •

edited

Loading

neolit123 left a comment •

edited

Loading

neolit123 Mar 3, 2025 •

edited

Loading

neolit123 commented Mar 3, 2025 •

edited

Loading