That makes sense to me. @vdusek, would you be interested in filing an issue and working on this since you were previously working on the sync_mode stuff?
David On Tue, Aug 28, 2018 at 5:06 PM Brian Bouterse <bbout...@redhat.com> wrote: > I couldn't come up with any more use cases that would motivate keeping > sync_mode over mirror. As pointed out, even retention would likely be its > own option. I'm now +1 on making the API and DeclarativeVersion use > mirror=False. > > I think I was the main dissenting opinion on this change, so I believe > this represents consensus. If there are any -1's please raise them on-list. > Otherwise I would say that PR and any follow-up work can go ahead and > happen. > > On Tue, Aug 21, 2018 at 12:48 PM, David Davis <davidda...@redhat.com> > wrote: > >> I wanted to bump this thread. There’s a PR waiting on a resolution. I see >> some agreement but not sure what the next steps are? >> >> David >> >> >> On Thu, Aug 9, 2018 at 5:34 PM Jeff Ortel <jor...@redhat.com> wrote: >> >>> >>> >>> 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> >>> 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> >>>> wrote: >>>> >>>>> >>>>> >>>>> On Wed, Aug 8, 2018 at 7:54 PM, Jeff Ortel <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 >>>>> [2] https://github.com/pulp/pulp/pull/3583#discussion_r208869824 >>>>> >>>>> >>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> 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 >>>>> >>>>> >>>> >>>> _______________________________________________ >>>> Pulp-dev mailing list >>>> Pulp-dev@redhat.com >>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>> >>>> >>> >>> >>> _______________________________________________ >>> Pulp-dev mailing >>> listPulp-dev@redhat.comhttps://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 >> >> >
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
