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.

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

Deprecate secret CLI param requirement from all commands but "server" #1544

Closed
wants to merge 1 commit into from

Conversation

paskal
Copy link
Sponsor Collaborator

@paskal paskal commented Dec 3, 2022

It was planned to be used in the import, restore, and backup commands, but later, Umputun ditched that idea. After that, one less CLI option to set for all the commands other than server.

Backwards compatibility preserved: providing the secret won't fail the command execution.

@paskal paskal added the backend label Dec 3, 2022
@paskal paskal added this to the v1.11.2 milestone Dec 3, 2022
@paskal paskal requested a review from umputun as a code owner December 3, 2022 10:39
@paskal paskal modified the milestones: v1.11.2, v1.11.3, v1.12.0 Jan 3, 2023
@paskal paskal force-pushed the paskal/remove_common_shared_secret branch from 7fab1cf to 2eb011e Compare January 3, 2023 18:02
@paskal paskal force-pushed the paskal/remove_common_shared_secret branch 2 times, most recently from 1713856 to 9c9ff5b Compare January 22, 2023 10:12
@paskal paskal force-pushed the paskal/remove_common_shared_secret branch from 9c9ff5b to db0d90c Compare February 5, 2023 20:13
@paskal paskal force-pushed the paskal/remove_common_shared_secret branch from db0d90c to 19bd259 Compare March 11, 2023 00:18
@paskal paskal force-pushed the paskal/remove_common_shared_secret branch 2 times, most recently from ed7ab7b to ffc817a Compare April 2, 2023 19:21
@paskal paskal force-pushed the paskal/remove_common_shared_secret branch from ffc817a to acf1a13 Compare May 1, 2023 21:25
@paskal paskal force-pushed the paskal/remove_common_shared_secret branch 2 times, most recently from 615c92a to ffbc3b1 Compare July 4, 2023 03:59
It was planned to be used in the import, restore, and backup commands,
but later, Umputun ditched that idea. After that, one less CLI option
to set for all the commands other than `server`.

Backwards compatibility preserved: providing the secret won't fail
the command execution.
@paskal paskal force-pushed the paskal/remove_common_shared_secret branch from ffbc3b1 to b370cfd Compare July 4, 2023 04:03
@paskal paskal changed the title Remove secret CLI param requirement from all commands but "server" Deprecate secret CLI param requirement from all commands but "server" Jul 4, 2023
@umputun
Copy link
Owner

umputun commented Jul 4, 2023

Still, I'm not too fond of it. You are probably correct - this change won't break back compatibility, but it adds extra complexity, and I wonder why we care about it. Keeping a shared secret field in CommonOpts doesn't hurt anything, but all the code to handle DeprecatedSharedSecret (needed for back compact) feels like something artificial and unnatural to me.

I'd say - let's keep it in CommonOpts, and if you think the presence of this flag is causing some confusion for a reader - we can add a comment saying, "currently used by ServerCommand only and ignored by others".

Generally, I agree with you that this flag is not common anymore, however, we want to keep it back compact, and the best way to do it is to change nothing and accept the fact - it is still here and (maybe) will have a use by some other commands some day.

@paskal paskal closed this Jul 6, 2023
@paskal paskal deleted the paskal/remove_common_shared_secret branch July 6, 2023 09:06
@paskal paskal removed this from the v1.12.0 milestone Jul 6, 2023
paskal added a commit that referenced this pull request Jul 6, 2023
It's not used aside from `server` but it was decided to keep it in place
for backwards compatibility in PR #1544.
umputun pushed a commit that referenced this pull request Jul 17, 2023
It's not used aside from `server` but it was decided to keep it in place
for backwards compatibility in PR #1544.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants