__timezone should be singleton #42

New Issue

ungleich-gitea · 2021-11-20T11:25:06Z

ungleich-gitea commented

2021-11-20 11:25:06 +00:00

The __timezone type should IMHO be a singleton type because it makes no sense to set multiple timezones (I don't know any operating systems on which this would even work.)

Such a change cannot be done without breaking current setups.
Would a MR that converts __timezone into a singleton type be accepted?

The `__timezone` type should IMHO be a singleton type because it makes no sense to set multiple timezones (I don't know any operating systems on which this would even work.) Such a change cannot be done without breaking current setups. Would a MR that converts `__timezone` into a singleton type be accepted?

ungleich-gitea closed this issue

2021-11-20 11:25:06 +00:00

ungleich-gitea commented

2021-11-20 15:29:54 +00:00

closed

ungleich-gitea commented

2021-11-20 15:29:55 +00:00

Since no one could really come up with a good solution to the transition issue, do you think we can go forward with !916 and add a "breaking changes" section to the changelog?

e.g.

Changelog
---------

next:
	**BREAKING CHANGES:**
		* Type __pf_apply: Remove deprecated type (Darko Poljak)
		* Type __timezone: Converted to singleton type (Dennis Camera)
		  The __timezone type is now a singleton which requires changes in code
		  using it.
		  Instead of:
		    __timezone Europe/Zurich
		  use:
		    __timezone --tz Europe/Zurich

	Features:
		* New type: __download (Ander Punnar)
		* New type: __unpack (Ander Punnar)
		* Type __hosts: Add --alias parameter (Dennis Camera)
		* Type __openldap_server: Add Alpine support (Timothée Floure)
		* Type __locale_system: Support more OSes (Dennis Camera)

	Fixes:
		* Type __package_opkg: Add locking (Dennis Camera)
		* Type __user: Fix shadow explorer for OpenBSD (Dennis Camera)
		* Core: Make emulator-part code consistent; remove faulty warning (Darko Poljak)
		* Types __file, __directory: Support setuid, setguid, sticky bits (Dennis Camera)
		* Type __postfix_master: Fix --option parameter and option expansion (Daniel Fancsali)
		* Type __user: Install user packages on OpenWrt (Dennis Camera)
		* Type __package_apt: Fix for legacy APT versions that do not support --no-install-recommends (Dennis Camera)
		* Type __key_value: Get awk from POSIX PATH (Dennis Camera)

Since no one could really come up with a good solution to the transition issue, do you think we can go forward with !916 and add a "breaking changes" section to the changelog? e.g. ``` Changelog --------- next: **BREAKING CHANGES:** * Type __pf_apply: Remove deprecated type (Darko Poljak) * Type __timezone: Converted to singleton type (Dennis Camera) The __timezone type is now a singleton which requires changes in code using it. Instead of: __timezone Europe/Zurich use: __timezone --tz Europe/Zurich Features: * New type: __download (Ander Punnar) * New type: __unpack (Ander Punnar) * Type __hosts: Add --alias parameter (Dennis Camera) * Type __openldap_server: Add Alpine support (Timothée Floure) * Type __locale_system: Support more OSes (Dennis Camera) Fixes: * Type __package_opkg: Add locking (Dennis Camera) * Type __user: Fix shadow explorer for OpenBSD (Dennis Camera) * Core: Make emulator-part code consistent; remove faulty warning (Darko Poljak) * Types __file, __directory: Support setuid, setguid, sticky bits (Dennis Camera) * Type __postfix_master: Fix --option parameter and option expansion (Daniel Fancsali) * Type __user: Install user packages on OpenWrt (Dennis Camera) * Type __package_apt: Fix for legacy APT versions that do not support --no-install-recommends (Dennis Camera) * Type __key_value: Get awk from POSIX PATH (Dennis Camera) ```

ungleich-gitea commented

2021-11-20 15:29:57 +00:00

mentioned in merge request !916

ungleich-gitea commented

2021-11-20 15:29:58 +00:00

This would require cdist core code changes, only for this one type.
What is deprecated here then?
Object id? Or better, type nature?

This would require cdist core code changes, only for this one type. What is deprecated here then? Object id? Or better, type nature?

ungleich-gitea commented

2021-11-20 15:29:59 +00:00

No, I thought that in the transition, you can use the type like this:

__timezone Europe/Berlin
__timezone --timezone Europe/Berlin

With the first call, it's map the "object id" to the parameter --timezone and echos a deprecation warning. After the deprecation is over, only the second call can be used.

With this, the "screw up" of the type system only happens in the transition phase and offers a smooth transition for users.

No, I thought that in the transition, you can use the type like this: ```sh __timezone Europe/Berlin __timezone --timezone Europe/Berlin ``` With the first call, it's map the "object id" to the parameter `--timezone` and echos a deprecation warning. After the deprecation is over, only the second call can be used. With this, the "screw up" of the type system only happens in the transition phase and offers a smooth transition for users.

ungleich-gitea commented

2021-11-20 15:30:01 +00:00

@matze But what should the new type be called?

ungleich-gitea commented

2021-11-20 15:30:02 +00:00

What's about to only use it while the non-singleton type is deprecated? So you can map the object id to one parameter. Then, every time the singleton type will be called as a normal object, it will map the object id and print a deprecation warning.

After a time, the mapping can be removed. It will be smoother than a temporary type and maybe a lesser hack than generally support an positional argument for singletons. Without a warning, it may be confused why singletons have an object id.

What's about to only use it while the non-singleton type is deprecated? So you can map the object id to one parameter. Then, every time the singleton type will be called as a normal object, it will map the object id and print a deprecation warning. After a time, the mapping can be removed. It will be smoother than a temporary type and maybe a lesser hack than generally support an positional argument for singletons. Without a warning, it may be confused why singletons have an object id.

ungleich-gitea commented

2021-11-20 15:30:03 +00:00

When I think more about it we shouldn't hack singleton type core support.
But we should decide on the way of deprecation and replacing the old one with the new one.

When I think more about it we shouldn't _hack_ singleton type core support. But we should decide on the way of deprecation and replacing the old one with the new one.

ungleich-gitea commented

2021-11-20 15:30:04 +00:00

With singleton types you cannot call it object id, since there can be only one singleton type object.

ungleich-gitea commented

2021-11-20 15:30:06 +00:00

I know, it's kind of a hack, but there is already only one positional argument allowed for non-singleton types, and that's object id.

I know, it's kind of a hack, but there **is** already only one positional argument allowed for non-singleton types, and that's object id.

ungleich-gitea commented

2021-11-20 15:30:07 +00:00

So there would only be one positional argument allowed and only for singleton types?

ungleich-gitea commented

2021-11-20 15:30:09 +00:00

@nico Hm... what if we can support positional parameter for singleton types? It wouldn't be object id but plain positional parameter, optional.
It's just that we would need to support it with some env var like object id is currently supported, like __singleton_arg. Does this sound good to you?
This way existing __timezone would not break current configurations.
It could also simplify/make more natural some future singleton types that require one argument.

@nico Hm... what if we can support positional parameter for singleton types? It wouldn't be object id but plain positional parameter, optional. It's just that we would need to support it with some env var like object id is currently supported, like `__singleton_arg`. Does this sound good to you? This way existing `__timezone` would not break current configurations. It could also simplify/make more natural some future singleton types that require one argument.

ungleich-gitea commented

2021-11-20 15:30:10 +00:00

but can't we allow object_id-s with singletons?

ungleich-gitea commented

2021-11-20 15:30:11 +00:00

The problem here is, we cannot really offer a transition path to the users. We can warn that it will break and then.. what? I have to continue using non-singleton until upstream switched to singleton.

So I certainly will be broken and I cannot do anything. That is not a good user experience.

What we could do is something like this:

introduce __timezonetmp as a singleton
warn users, advocate to migrate to __timezonetmp
Then we will have many users potentially with __timezonetmp
We modify __timezone
We deprecate again
Users have to follow up again

Also not a great user experience.

I wonder if we can offer something better, because this is basically a non-feature change that will cause people to migrate.

We could also say we copy __timezone to __timezone_another_name as a singleton and make this the default one.

Anyway, you get the picture.

The problem here is, we cannot really offer a transition path to the users. We can warn that it will break and then.. what? I have to continue using non-singleton until upstream switched to singleton. So I certainly will be broken and I cannot do anything. That is not a good user experience. What we could do is something like this: * introduce `__timezonetmp` as a singleton * warn users, advocate to migrate to `__timezonetmp` * Then we will have many users potentially with __timezonetmp * We modify __timezone * We deprecate again * Users have to follow up again Also not a great user experience. I wonder if we can offer something better, because this is basically a non-feature change that will cause people to migrate. We could also say we copy `__timezone` to `__timezone_another_name` as a singleton and make this the default one. Anyway, you get the picture.

ungleich-gitea commented

2021-11-20 15:30:13 +00:00

Maybe we should queue the change for an upcoming 7.0.0 release?
IMO breaking API changes are okay for major releases.

Maybe we should queue the change for an upcoming 7.0.0 release? IMO breaking API changes are okay for major releases.

ungleich-gitea commented

2021-11-20 15:30:14 +00:00

You are right, it should have been a singleton. I'm not sure what's the
best path here for change, as we would need a second type if we want to
give users the opportunity to modify their code without breaking.

You are right, it should have been a singleton. I'm not sure what's the best path here for change, as we would need a second type if we want to give users the opportunity to modify their code without breaking.

ungleich-gitea commented

2021-11-20 15:30:15 +00:00

Hm...didn't think about the object id.
Singleton change which requires parameter would break existing configurations.
We should probably go with deprecation and eventually change it to something like __timezone --tz Europe/Talling?
@nico What is your opinion on this one?

Hm...didn't think about the object id. Singleton change which requires parameter would break existing configurations. We should probably go with deprecation and eventually change it to something like `__timezone --tz Europe/Talling`? @nico What is your opinion on this one?

ungleich-gitea commented

2021-11-20 15:30:17 +00:00

I think it was done probably because it's nicer to write __timezone Europe/Tallinn vs __timezone --timezone Europe/Tallinn.

maybe allow object_id if singleton? @poljakowski

I think it was done probably because it's nicer to write `__timezone Europe/Tallinn` vs `__timezone --timezone Europe/Tallinn`. maybe allow object_id if singleton? @poljakowski

ungleich-gitea commented

2021-11-20 15:30:18 +00:00

yes

Sign in to join this conversation.