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
