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

better management of mixing flags and config #2902

Open
2 of 5 tasks
chendave opened this issue Jul 10, 2023 · 7 comments
Open
2 of 5 tasks

better management of mixing flags and config #2902

chendave opened this issue Jul 10, 2023 · 7 comments
Labels
area/cli kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/design Categorizes issue or PR as related to design. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Milestone
v1.33

Comments

@chendave
Copy link
Member

chendave commented Jul 10, 2023 •
edited by neolit123
Loading

What happened?

pls see: kubernetes/kubernetes#113583 (comment)

the major reason to against the mix configuration is:

migrate users away from flags

but it is not clear why some of them are allowed for the mix configuration in the first place.

https://github.com/kubernetes/kubernetes/blob/0ae9aaacfa217b18652f34d6ef29f08a15851b0a/cmd/kubeadm/app/apis/kubeadm/validation/validation.go#L560-L573

func isAllowedFlag(flagName string) bool {
	allowedFlags := sets.New(kubeadmcmdoptions.CfgPath,
		kubeadmcmdoptions.IgnorePreflightErrors,
		kubeadmcmdoptions.DryRun,
		kubeadmcmdoptions.KubeconfigPath,
		kubeadmcmdoptions.NodeName,
		kubeadmcmdoptions.KubeconfigDir,
		kubeadmcmdoptions.UploadCerts,
		"print-join-command", "rootfs", "v", "log-file")
	if allowedFlags.Has(flagName) {
		return true
	}
	return strings.HasPrefix(flagName, "skip-")
}

What you expected to happen?

  • Kubeadm should have a consistent style for the configuration
  • If this is not possible, Kubeadm should make it clear why some of the flags are allowed the mix configuration, while other not.

How to reproduce it (as minimally and precisely as possible)?

Anything else we need to know?

Tasks
Beta Give feedback
@chendave chendave mentioned this issue Jul 10, 2023
Merged
@neolit123
Copy link
Member

but it is not clear why some of them are allowed for the mix configuration in the first place.

  • some flags exist as the only option and have no alternative in config, e.g. --dry-run
  • some flags are not deprecated yet; also need to be removed, but the deprecatation takes 1 year+ as the CLI is GA.

@neolit123 neolit123 added kind/design Categorizes issue or PR as related to design. area/cli labels Jul 10, 2023
@neolit123 neolit123 added this to the v1.28 milestone Jul 10, 2023
@neolit123 neolit123 added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jul 10, 2023
@neolit123
Copy link
Member

neolit123 commented Jul 10, 2023 •
edited
Loading

my vote, with respect to reset's --config:

  • we can allow the mixture of all options that don't exist in the reset config
  • all flag options that also exists in config should not be allowed - i.e. kubeadm should error saying "flag not allowed with --config mix" - e.g. --skip-phases

@chendave chendave mentioned this issue Jul 10, 2023
Closed
@chendave
Copy link
Member Author

some flags exist as the only option and have no alternative in config, e.g. --dry-run

Added a TODO item in the issue: #2890

@SataQiu
Copy link
Member

SataQiu commented Jul 10, 2023

  • we can allow the mixture of all options that don't exist in the reset config
  • all flag options that also exists in config should not be allowed - i.e. kubeadm should error saying "flag not allowed with --config mix" - e.g. --skip-phases

I'm +1 for this.
All fields that can be defined in the configuration file should no longer be allowed to be specified on the CLI flags, and we prefer to guide users to use the configuration file instead of relying too heavily on the CLI flags.

@chendave chendave mentioned this issue Jul 13, 2023
Merged
@neolit123
Copy link
Member

if we don't enumerate some action items here, perhaps we should just close? the story is clear, it feels like - @chendave as this ticket was more of a discussion / question.

biggest blocker of course is that the cli is ga, and all non experimental flags must take one year deprecation period.

@chendave
Copy link
Member Author

@neolit123 I added some action items, I'd suggest to keep this open so won't forget to fix it in the 1.29.

@neolit123 neolit123 modified the milestones: v1.28, v1.29 Jul 21, 2023
@CecileRobertMichon CecileRobertMichon mentioned this issue Aug 24, 2023
Closed
@neolit123 neolit123 modified the milestones: v1.29, v1.30 Nov 1, 2023
@neolit123 neolit123 changed the title mixed config with flags and config file, should kubeadm allow this? better management of mixing flags and config Nov 30, 2023
@neolit123 neolit123 added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. labels Nov 30, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 28, 2024
@neolit123 neolit123 added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 28, 2024
@neolit123 neolit123 modified the milestones: v1.30, v1.31 Apr 5, 2024
@neolit123 neolit123 modified the milestones: v1.31, v1.32 Aug 7, 2024
@neolit123 neolit123 removed this from the v1.32 milestone Nov 27, 2024
@neolit123 neolit123 added this to the v1.33 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/cli kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/design Categorizes issue or PR as related to design. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Milestone
v1.33
Development

No branches or pull requests
5 participants
@neolit123 @chendave @SataQiu @k8s-ci-robot @k8s-triage-robot