On 08/09/2018 01:29 PM, Daniel Alley wrote:

    It's possible we could want additional sync_modes in the future.
    To me, sync mode deals with the contents of the repo during the
    sync. There are other ways you would want to have a sync associate
    content with a repository. Consider a retention behavior that
    retains 5 versions of each unit, e.g. rpms, ansible modules, etc;
    that behavior is somewhere in between mirror and additive. If we
    make mirror a boolean then to introduce this retention feature we
    would have to add it as an additional option. This creates the
    downside I hope to avoid which is that interaction between options
    becomes complicated.

    For example, a call with both (mirror=False, retention=True) now
    becomes more complicated to think about. Is it mirroring or using
    the retention policy? How do these interact? At that point, it
    seems more complicated than what we have now. The way to avoid
    this is by keeping them together as one option, but that can only
    be done if it stays as a string.


These are all good points but I think "retention" would likely need to be a configurable parameter, probably one that you would have to pass in.  The default value could mean "unlimited retention", i.e.  "additive".

So what you could do is:

    (mirror=False)                       # this is normal additive
    mode, retain everything.  let's say that default retention=0,
    which is nonsensical and would map to this behavior instead

    (mirror=False, retention=5)     # retain at most 5 versions of any
    given unit

    (mirror=False, retention=1)     # this is *almost* like mirror
    mode, except that you would still keep one historical copy of
    units that are no longer present in the upstream repository


Maybe it even makes sense to have retention be able to modify "mirror" mode, although this would make the concept of "mirror" more difficult to understand as you point out.  Maybe we could find a name that would be less misleading.

    (mirror=True, retention=5)       # retain at most 5 versions of
    any given unit, /but purge units that that are no longer present
    in the upstream repo entirely/


This ^^ matches what I was thinking as well.


I don't have a specific use case in mind for that one, but maybe someone can think of one?


On Thu, Aug 9, 2018 at 12:53 PM, Brian Bouterse <bbout...@redhat.com <mailto:bbout...@redhat.com>> wrote:

    It's possible we could want additional sync_modes in the future.
    To me, sync mode deals with the contents of the repo during the
    sync. There are other ways you would want to have a sync associate
    content with a repository. Consider a retention behavior that
    retains 5 versions of each unit, e.g. rpms, ansible modules, etc;
    that behavior is somewhere in between mirror and additive. If we
    make mirror a boolean then to introduce this retention feature we
    would have to add it as an additional option. This creates the
    downside I hope to avoid which is that interaction between options
    becomes complicated.

    For example, a call with both (mirror=False, retention=True) now
    becomes more complicated to think about. Is it mirroring or using
    the retention policy? How do these interact? At that point, it
    seems more complicated than what we have now. The way to avoid
    this is by keeping them together as one option, but that can only
    be done if it stays as a string.

    On Thu, Aug 9, 2018 at 9:04 AM, Milan Kovacik <mkova...@redhat.com
    <mailto:mkova...@redhat.com>> wrote:



        On Wed, Aug 8, 2018 at 7:54 PM, Jeff Ortel <jor...@redhat.com
        <mailto:jor...@redhat.com>> wrote:

            I'm not convinced that /named/ sync mode is a good
            approach.  I doubt it will ever be anything besides
            (additive|mirror) which really boils down to mirror (or
            not).  Perhaps the reasoning behind a /named/ mode is that
            it is potentially more extensible in that the API won't be
            impacted when a new mode is needed. The main problem with
            this approach is that the mode names are validated and
            interpreted in multiple places. Adding another mode will
            require coordinated changes in both the core and most
            plugins.  Generally, I'm an advocate of named things like
            /modes/ and /policies/ but given the orthogonal nature of
            the two modes we currently support _and_ that no /real/ or
            anticipated use cases for additional modes are known, I'm
            not convinced it's a good fit.  Are there any /real/ or
            anticipated use cases I'm missing?


        Looking at the code[1] we're actually talking about almost a
        (pipeline) factory that has exactly 2 modes of operation with
        a limited possibilities of extending, unsure that the
        possibility to extend was a goal though.
        Moreover it turns out current implementation prevents using
        (class-level) constants instead of custom strings due to
        plugin--platform import issues: core serializer can't refer to
        DeclarativeVersion.defaul_sync_mode --- at least I wasn't able
        to make this work as part of the sync_mode docstring PR[2]
        review suggestion.


            I propose we replace the (str)sync_mode="" with
            (bool)mirror=False anywhere stored or passed.

            Are there any /real/ or anticipated use cases I'm missing?

            Thoughts?


        I'm afraid replacing custom strings with True/False won't make
        the situation much better.
        I'd vote for some refactor besides other things, it might
        better be part of remote (or repository) creation endpoint.

        Cheers,
        milan
        [1]
        
https://github.com/pulp/pulp/blob/master/plugin/pulpcore/plugin/stages/declarative_version.py#L100#L114
        
<https://github.com/pulp/pulp/blob/master/plugin/pulpcore/plugin/stages/declarative_version.py#L100%23L114>
        [2]
        https://github.com/pulp/pulp/pull/3583#discussion_r208869824
        <https://github.com/pulp/pulp/pull/3583#discussion_r208869824>




            _______________________________________________
            Pulp-dev mailing list
            Pulp-dev@redhat.com <mailto:Pulp-dev@redhat.com>
            https://www.redhat.com/mailman/listinfo/pulp-dev
            <https://www.redhat.com/mailman/listinfo/pulp-dev>



        _______________________________________________
        Pulp-dev mailing list
        Pulp-dev@redhat.com <mailto:Pulp-dev@redhat.com>
        https://www.redhat.com/mailman/listinfo/pulp-dev
        <https://www.redhat.com/mailman/listinfo/pulp-dev>



    _______________________________________________
    Pulp-dev mailing list
    Pulp-dev@redhat.com <mailto:Pulp-dev@redhat.com>
    https://www.redhat.com/mailman/listinfo/pulp-dev
    <https://www.redhat.com/mailman/listinfo/pulp-dev>




_______________________________________________
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

_______________________________________________
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev

Reply via email to