Add optional attribute 'replaced-by' for parameters/parameter

[marxsk]

Re-opening on @kgaillot request. This feature is targeting for 1.2+ release

[kgaillot]

Is 'replaced-by' already in use anywhere?

Is the intent to indicate that the parameter has been deprecated in favor of a new parameter name?

[marxsk]

@kgaillot yes, fence agents have this functionality but the argument names are more confusing (obsoletes/deprecated)

[kgaillot]

There was discussion at one point of having a "status" for the agent as a whole, to indicate deprecated, experimental, etc. Perhaps we need something like that for parameters as well? I remember at one point we also wanted to mark parameters as advanced.

Is there a reason we need the new parameter name here, rather than just deprecated=(0|1)?

[krig]

With a link to the new name, the UI can provide more help when someone configures using the deprecated parameter. Though ideally you would want even more information in these cases... maybe something like a deprecated element that can have its own description would be even better...

<parameter>
  <deprecated replaced-by="...">
    <desc lang="en">
      This parameter has been deprecated in favor of foo, which supports the new syntax introduced in version 3.0.
    </desc>
  </deprecated>
</parameter>

[krig]

Though I have to say that personally I would prefer it if agents never deprecated or replaced parameter names to preserve backwards compatibility.

[marxsk]

@kgaillot yes, we would like to mark 'level' of the parameter as well. It might be good idea to merge it together but as @krig said from UI perspective it helps a lot.

@krig I agree that this should not happen very often but in fence-agents we have inconsistency between argument on command-line and STDIN for last 10 years. And after this, we will be able to remove it what would simplify things a lot.

[kgaillot]

@krig I think longdesc is a good place for such info -- it might be confusing for a UI to show one description in one place and another one elsewhere.

@krig @marxsk Do UIs need the new name directly, or would it be sufficient for the author to mention it in the longdesc?

I'm thinking something like status="(experimental|deprecated|expert|normal)" where UIs would only show "normal" options unless the user selects something like "advanced". E.g.

<parameter name="widget" status="deprecated">
     <longdesc lang="en">
          The thingamajiggy to frobnicate. This parameter has been deprecated in favor of foo.
     </longdesc>
</parameter>

[marxsk]

@kgaillot

I would prefer to have the new name accessible. As we will have to work also with older configurations that will contain deprecated arguments. We have to show them if they are set. Having the names will help in order to show it together (or propose migration to new ones)

attribute state) your idea makes sense and having 'normal/advanced/..' will also help in UI

[jnpkrn]

Please save my eyes: s/replaced-by/replaced-with/ if need be.

Regarding status, yes, that's an idea spanning way too many years.

[krig]

The benefit of having the name outside the description is that the UI can do things like show a link from the deprecated parameter to the new one, and vice versa show a hint on the new parameter that it deprecates the older one. Strict validation can check for usage of deprecated variables and signal the user.

@kgaillot If the deprecation description is separate from the parameter longdesc, the UI can do things like show a "deprecated" link which when hovered or clicked shows the details. Having too much text in the longdesc clutters the description, and people tend to glaze over long paragraphs of text, so it ultimately defeats the purpose of the description to begin with. So I would be in favor of more granular semantics in the agent metadata.

[krig]

@jnpkrn "replaced by" and "replaced with" are both grammatically correct :)

[krig]

Oh, and I would also vote in favor of a status tag, that's a great idea. Maybe even with a "replaced by/with" attribute for the whole agent, as well? That would also help with a future renaming of agents like IPaddr2 to something like ip-address for example, or with the LVM agent naming debacle...

[krig]

For status as an attribute on parameters, how about calling it flags and making it a list? So that a parameter can be both expert and deprecated, for example.

[kgaillot]

Oh, and I would also vote in favor of a status tag, that's a great idea. Maybe even with a "replaced by/with" attribute for the whole agent, as well? That would also help with a future renaming of agents like IPaddr2 to something like ip-address for example, or with the LVM agent naming debacle...

We do need some mechanism to handle agent deprecation/renaming as well, but that has additional impact. If the goal is to have a UI warn a user when they attempt to use a deprecated agent, then meta-data in the deprecated agent is fine. But if we want more significant aliasing, e.g. to have a stub for the deprecated agent that just points to the new name, then we have to figure out how both UIs and resource managers would be able to use that.

[kgaillot]

For status as an attribute on parameters, how about calling it flags and making it a list? So that a parameter can be both expert and deprecated, for example.

I considered that, but I think it's overkill. If an expert parameter is deprecated, then it's no longer expert -- it's deprecated. I think the usage of it will be to group parameters (rather than label them individually), so a single heading makes more sense. E.g. the main screen only shows the "normal" parameters, and if the user clicks "Advanced...", a new screen with parameters grouped under Expert/Experimental/Deprecated headings is shown.

Also, to take advantage of XML's structure, we should implement flags as individual boolean attributes (e.g. experimental=0 deprecated=1 expert=1) if we do go that way.

[krig]

Also, to take advantage of XML's structure, we should implement flags as individual boolean attributes (e.g. experimental=0 deprecated=1 expert=1) if we do go that way.

That sounds like a good solution to me. expert and deprecated to me seem like orthogonal concepts, and to say that these flags would only be useful to sort parameters into different categories doesn't make sense. For example, I would naively think to implement a UI so that parameters marked "expert" are hidden behind an "advanced options" click-through, but I would mark deprecated parameters with an icon or warning label while keeping them in the category where they were before they were deprecated.

[jnpkrn]

On 29/03/18 08:20 +0000, Kristoffer Grönlund wrote:

@jnpkrn "replaced by" and "replaced with" are both grammatically correct :)

I was concerned about semantics and active/passive actor dichotomy. Unless we want to track replaced-by=e-ddie :-)

-- Jan (Poki)

[marxsk]

@krig: yes, that makes sense. Our new GUI is using this concept as well.

[kgaillot]

To recap the unresolved questions here:

1. Do we want to change the attribute name from replaced-by to something else (e.g. replaced-with, obsoleted-by, use-instead)?

2. Do we need a new element with an explanation of why it is deprecated (e.g. deprecated-desc or something like @krig's first comment)?

3. Do we want to add experimental/deprecated/expert attributes in this PR or leave it for another day?

[marxsk]

@kgaillot

1) I'm okay with replaced-by, replaced-with and obsoleted-by

2) I would not add it, we can update longdesc in this case

3) Add

[jnpkrn]

Still a replaced-with proponent :-)

To combine that with an example from practice, I'll further instigate that the same designation is used also for parameters dropped without replacement, which seems currently likely with configdir of ocf:pacemaker:controld: https://github.com/ClusterLabs/pacemaker/blob/Pacemaker-2.0.0-rc2/extra/resources/controld#L67

That parameter has never been "grounded" so that tool under control would follow it, hence it has basically been spurious since its introduction (dlm maintainer is currently not inclined to accept the additional variability).

Hence the solution would be to declare

<parameter name="configdir" unique="1" replaced-with="">

[jnpkrn]

(disclosure: this semantic overloading inspired with https://github.com/ClusterLabs/pacemaker/blob/Pacemaker-2.0.0-rc2/xml/upgrade-2.10.xsl#L30-L33)

[kgaillot]

1. I'm okay with replaced-by, replaced-with and obsoleted-by

I'm wondering about the case @jnpkrn mentioned, where an attribute has been deprecated with no replacement. I don't think an empty value is good because it's not apparent whether that would mean "obsoleted with no replacement" or "not obsoleted". It's quite common for programs to dump an array of all possible parameters, leaving those blank that are not otherwise specified, which would have an unintended effect if we distinguish "not present" from "present but empty".

It may also be useful to consider the case where one parameter has been obsoleted by multiple new ones, say a server=host:port parameter that has been replaced with separate host and port parameters.

What about:

<parameter name="foo" deprecated="true">
  <replaced-with name="bar"/>
  <replaced-with name="thingy"/>
</parameter>

2. I would not add it, we can update longdesc in this case

3. Add

Are you going to add a commit with those?

[marxsk]

@kgaillot

I understand those changes in format but in such case, I will just explain it in the longdesc. I'm not against special flag/attribute that will show that there is no replacement.

[kgaillot]

I forgot to indent my code block so it disappeared, edited previous comment :)

[kgaillot]

@marxsk If I follow you correctly, you would prefer not to have multiple replaced fields? Incorporating @krig's initial suggestion, what about:

<parameter name="foo">
    <deprecated replaced-with="bar" lang="en">Don't use foo, it's bad.</deprecated>
</parameter>

where replaced-with is optional (missing meaning no replacement, or no hint for UIs at least) and so is the extended description (UIs can use it if present, or the longdesc if not). We could implement the other statuses similarly, e.g.

<advanced/>

so additional info could be added to them in the future if the need arises.

[marxsk]

@kgaillot I completely agree with the first solution. The only issue with might be an expension of used elements. Perhaps having the 'advanced' in an attribute would be enough.

[kgaillot]

@marxsk OK, then let's do the deprecated tag and leave the rest for a separate PR for discussion.

[krig]

I'm OK with the version you've settled on as well.

[jnpkrn]

On 01/05/18 09:03 -0700, Ken Gaillot wrote:

I'm wondering about the case @jnpkrn mentioned, where an attribute has been deprecated with no replacement. I don't think an empty value is good because it's not apparent whether that would mean "obsoleted with no replacement" or "not obsoleted".

In the intended way of use, it would be crystal clear: if the parameter wasn't obsoleted, it would simply refrain from stating obsoleted-with at all. In an XML sense, it would really mean a different thing than obsoleted-with="".

It's quite common for programs to dump an array of all possible parameters, leaving those blank that are not otherwise specified, which would have an unintended effect if we distinguish "not present" from "present but empty".

We are talking about metadata that are static for the most part, and if there is some convenience wrapper introduced, it would need to take this difference into account.

It may also be useful to consider the case where one parameter has been obsoleted by multiple new ones, say a server=host:port parameter that has been replaced with separate host and port parameters.

obsoleted-with="host+port" (plus sign cannot occur in the attribute name in XML and we'd rather somewhat constrain this in OCF, too, since it may not be very portable for the shell scenarios, one of the plentiful neglectings in the current standard)

On 03/05/18 14:24 +0000, Ken Gaillot wrote:

Incorporating @krig's initial suggestion, what about:

<parameter name="foo">
    <deprecated replaced-with="bar" lang="en">Don't use foo, it's bad.</deprecated>
</parameter>

This looks self-descriptive, which is good.

However, and this is more generic, the lang encoding really doesn't sit well here -- does it want to suggest that the deprecation depends on the language of the environment, like, are we allowing i18n inside out incl. parameters? :-)

If anybody's interested, I'd rather claim lang="en" implicit, and if there's an unstoppable desire to localize, have nested i18n elements:

<parameter name="foo">
    <deprecated replaced-with="bar">
        Don't use foo, it's bad.
        <i18n lang="cs">Nepoužívej foo, sic to schytáš</i18n>
        <i18n lang="sk">Nepoužívať foo, je voľaké čudné</i18n>
    </deprecated>
</parameter>

Metadata processor would rather grab parameter/deprecated/text()[1] and trim/strip it internally, indeed.

-- Jan (Poki)

[jnpkrn]

On 04/05/18 18:08 +0200, Jan Pokorný wrote:

On 01/05/18 09:03 -0700, Ken Gaillot wrote:

It may also be useful to consider the case where one parameter has been obsoleted by multiple new ones, say a server=host:port parameter that has been replaced with separate host and port parameters.

obsoleted-with="host+port" (plus sign cannot occur in the attribute name in XML and we'd rather somewhat constrain this in OCF, too, since it may not be very portable for the shell scenarios, one of the plentiful neglectings in the current standard)

Also it seems using arbitrary characters (even if limited to ASCII) would make pengine/schedulerd explode since parameters to the agents appear to be encoded as attribute-based assignments in the XML of the transition graph (!!!). Frustrating to see the basic data model is so underspecified.

And no, shell won't accept crazy variable names off the bat:

$ env "a+b=3" sh -c 'env | grep -F a+b; echo ${a+b}; echo $a+b'

a+b=3

+b

but on the other hand:

$ env "a+b=3" python -c 'from os import getenv; print(getenv("a+b"))'

3

-- Jan (Poki)

[marxsk]

2018-05-04 18:08 GMT+02:00 Jan Pokorný notifications@github.com:

It may also be useful to consider the case where one parameter has been obsoleted by multiple new ones, say a server=host:port parameter that has been replaced with separate host and port parameters.

obsoleted-with="host+port" (plus sign cannot occur in the attribute name in XML and we'd rather somewhat contrain this in OCF, too, since it may not be very portable for the shell scenarios, one of the plentiful neglectings in the current standard)

I'm against this 'host+port' solution, when we really need such strong formalism, we should use elements instead of attributes.

On 03/05/18 14:24 +0000, Ken Gaillot wrote:

Incorporating @krig's initial suggestion, what about:

Don't use foo, it's bad.

This looks self-descriptive, which is good.

However, and this is more generic, the lang encoding really doesn't sit well here -- does it want to suggest that the deprecation depends on the language of the environment, like, are we allowing i18n inside out incl. parameters? :-)

If anybody's interested, I'd rather claim lang="en" implicit, and if there's an unstoppable desire to localize, have nested i18n elements:

Don't use foo, it's bad. Nepoužívej foo, sic to schytáš Nepoužívať foo, je volaké čudné

This is nice idea but it should be done that way in all of the cases (e.g. including parameters). I suggest to at least think about it for next major release. Please open new PR for this feature.

[jnpkrn]

On 04/05/18 23:01 +0200, Jan Pokorný wrote:

On 04/05/18 18:08 +0200, Jan Pokorný wrote:

On 01/05/18 09:03 -0700, Ken Gaillot wrote:

It may also be useful to consider the case where one parameter has been obsoleted by multiple new ones, say a server=host:port parameter that has been replaced with separate host and port parameters.

obsoleted-with="host+port" (plus sign cannot occur in the attribute name in XML and we'd rather somewhat constrain this in OCF, too, since it may not be very portable for the shell scenarios, one of the plentiful neglectings in the current standard)

Also it seems using arbitrary characters (even if limited to ASCII) would make pengine/schedulerd explode since parameters to the agents appear to be encoded as attribute-based assignments in the XML of the transition graph (!!!). Frustrating to see the basic data model is so underspecified.

And no, shell won't accept crazy variable names off the bat:

$ env "a+b=3" sh -c 'env | grep -F a+b; echo ${a+b}; echo $a+b'

a+b=3

+b

but on the other hand:

$ env "a+b=3" python -c 'from os import getenv; print(getenv("a+b"))'

3

Opened https://github.com/ClusterLabs/OCF-spec/issues/19

-- Jan (Poki)

[kgaillot]

Having both text and sub-elements inside <deprecated> looks confusing and non-orthogonal to me, not to mention inconsistent with current i18n syntax, so I'm back to @krig's original approach:

Trivial case:

<parameter name="foo">
  <deprecated/>
</parameter>

A replacement can be specified:

<parameter name="foo">
  <deprecated replaced-with="bar"/>
</parameter>

And a description can be given in any language (defaulting to en):

<parameter name="foo">
  <deprecated replaced-with="bar">
    <desc>Don't use foo, it's bad.</desc>
    <desc lang="cs">Nepoužívej foo, sic to schytáš</desc>
  </deprecated>
</parameter>

If that sounds OK, @marxsk , please update this PR accordingly, and I'll merge it.

[marxsk]

Done, the only difference is that lang is required as before. We should remove that in separate PR as it should impact also all short/longdesc