-
Notifications
You must be signed in to change notification settings - Fork 1.6k
✨(alpha update): add --git-config flag with clear defaults and replacement behavior #5017
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.
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
✨(alpha update): add --git-config flag with clear defaults and replacement behavior #5017
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 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 |
4c85725 to
a4ae256
Compare
| Keep built-in defaults (renameLimit tweaks) AND add more by repeating them explicitly | ||
|
|
||
| ```shell | ||
| kubebuilder alpha update \ | ||
| --git-config merge.renameLimit=999999 \ | ||
| --git-config diff.renameLimit=999999 \ | ||
| --git-config merge.conflictStyle=diff3 \ | ||
| --git-config rerere.enabled=true | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86 This section may mislead the user into thinking that those configs in the codeblock are the defaults. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point I changed
Let me know wdyt now
a4ae256 to
aa48f76
Compare
|
c/c @vitorfloriano could you please review this one? |
| | `--git-config` | Repeatable. Pass per-invocation Git config as `-c key=value`. **Default** (if omitted): `-c merge.renameLimit=999999 -c diff.renameLimit=999999`. **Important**: if you pass this flag, your values **replace** the defaults. To keep them *and* add more, re-specify the defaults explicitly. | | ||
| | `-h, --help` | Show help for this command. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check that I understand this:
By default we set those two configs, right? If we pass any values to this flag, we won't have those two configs anymore, unless we re-add them along with the other configs we may pass.
Did I get it right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it a little to improve the UX
Give a look now in the docs changes and let me know wdyt?
See if you agree with for we get it merged
9b04ae3 to
dc36149
Compare
|
|
||
| - **Disable defaults entirely** | ||
| ```shell | ||
| kubebuilder alpha update --git-config disable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86 What if we keep the default values UNLESS the user explicitly overrides them?
I think that would make the defaults more robust, if we believe the defaults are the way to go. But if the user disagrees, they can set THAT config as they wish.
Having three/four behaviors for this flag seems a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86 What if we keep the default values UNLESS the user explicitly overrides them?
Yes
I think that would make the defaults more robust, if we believe the defaults are the way to go. But if the user disagrees, they can set THAT config as they wish.
Exactly.
Having three/four behaviors for this flag seems a bit confusing.
- we have not inform : that has the default values
- if set: then is default values + what is passed
- if want to disable set : disable
I agree that is not simple
But it is an very advanaced feature so people will not tough unless they understand
I could not came with a better ux than that, if you have ideas please feel free to share
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could keep the defaults regardless of the user adding different configs (ex, conflictStyle=diff3 won't remove the defaults).
The defaults would only be changed if the user explicitly does so.
That way we wouldn't need the "disable" option, and we wouldn't need to add the defaults back in just because we want other, non-related, configs as well.
And, since the configs will only be set for those specific commands, I think we can be a bit more assertive/invasive about the configs we think are the best for what we are doing in kubebuilder alpha update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could keep the defaults regardless of the user adding different configs (ex, conflictStyle=diff3 won't remove the defaults).
that is how it is.
The defaults would only be changed if the user explicitly does so.
That is how it is.
The user must add "disable"
So, not sure if I follow your idea
But we must provide a option to allow the person opt-out besides we want ensure those by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is how it is.
I understood that:
- There are two configs used by default
- If the user passes any values to
--git-config, the defaults will no longer apply, even if the values passed have nothing to do with the defaults. - If the user wants to pass values to
--git-configAND keep the two defaults, the defaults need to be passed again. - If the user wants to disable the defaults, they can pass
disable.
My suggestion was that:
- There are two configs used by default
- The defaults are kept even when the user passes other configs to
--git-config. - The defaults are only overriden if the user passes different values to the same configs
Let me know if I got anything wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user passes any values to --git-config, the defaults will no longer apply,
We changed that, see: https://github.com/kubernetes-sigs/kubebuilder/pull/5017/files#diff-3c12a0b0aee4383e0a39d64d93c5286c1f1b1384396e8fc067d284711ced078cR136
- If --git-config is not set: Then we have the defaults
- if --git-config is set: Then we have the defaults + values informed
- if I want to disable the defaults then I need : --git-config disable and if I wish add others I append --git-config
| | `--preserve-path` | Repeatable. **Squash mode only.** After copying the merge tree to the output branch, restore these paths from the base branch (e.g., `.github/workflows`). | | ||
| | `--output-branch` | Name of the output branch. Default: `kubebuilder-update-from-<from-version>-to-<to-version>`. | | ||
| | `--push` | Push the output branch to the `origin` remote after the update completes. | | ||
| | `--git-config` | Repeatable. Pass per-invocation Git config as `-c key=value`. **Default** (if omitted): `-c merge.renameLimit=999999 -c diff.renameLimit=999999`. **Important**: if you pass this flag, your values **replace** the defaults. To keep them *and* add more, re-specify the defaults explicitly. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix ^ Outdated
| | `--from-branch` | Git branch that holds your current project code. Defaults to `main`. | | ||
| | `--force` | Continue even if merge conflicts happen. Conflicted files are committed with conflict markers (CI/cron friendly). | | ||
| | `--show-commits` | Keep full history (do not squash). **Not compatible** with `--preserve-path`. | | ||
| | `--preserve-path` | Repeatable. **Squash mode only.** After copying the merge tree to the output branch, restore these paths from the base branch (e.g., `.github/workflows`). | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
restore-path ^
dc36149 to
259004a
Compare
|
I changed the the flag info in the doc. |
259004a to
032373d
Compare
|
/lgtm Nice addition for advanced users. |
|
@vitorfloriano: changing LGTM is restricted to collaborators In response to this:
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. |
…lacement behavior Assisted-by: ChatGPT (OpenAI) Co-authored-by: Vitor Floriano <vitorfloriano@users.noreply.github.com>
032373d to
1b89844
Compare
No description provided.