14:32:09 <fao89> #startmeeting Pulp Triage 2020-04-28
14:32:09 <fao89> !start
14:32:09 <fao89> #info fao89 has joined triage
14:32:09 <pulpbot> Meeting started Tue Apr 28 14:32:09 2020 UTC.  The chair is fao89. Information about MeetBot at http://wiki.debian.org/MeetBot.
14:32:09 <pulpbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
14:32:09 <pulpbot> The meeting name has been set to 'pulp_triage_2020-04-28'
14:32:09 <pulpbot> fao89: fao89 has joined triage
14:32:17 <ppicka> #info ppicka has joined triage
14:32:17 <ppicka> !here
14:32:17 <pulpbot> ppicka: ppicka has joined triage
14:32:19 <daviddavis> #info daviddavis has joined triage
14:32:19 <daviddavis> !here
14:32:19 <pulpbot> daviddavis: daviddavis has joined triage
14:32:20 <mikedep333> #info mikedep333 has joined triage
14:32:20 <mikedep333> !here
14:32:21 <dalley> dkliban, I'll respond after triage
14:32:21 <pulpbot> mikedep333: mikedep333 has joined triage
14:32:24 <dalley> I have some thoughts
14:32:25 <dalley> #info dalley has joined triage
14:32:25 <dalley> !here
14:32:25 <pulpbot> dalley: dalley has joined triage
14:32:26 <x9c4> #info x9c4 has joined triage
14:32:26 <x9c4> !here
14:32:27 <pulpbot> x9c4: x9c4 has joined triage
14:32:28 <dkliban> #info dkliban has joined triage
14:32:28 <dkliban> !here
14:32:28 <pulpbot> dkliban: dkliban has joined triage
14:32:30 <fao89> !next
14:32:31 <pulpbot> fao89: 4 issues left to triage: 6586, 6583, 6581, 6569
14:32:31 <fao89> #topic https://pulp.plan.io/issues/6586
14:32:32 <pulpbot> RM 6586 - mdepaulo@redhat.com - NEW - pulpcore-api.service fails to start on containers running systemd
14:32:33 <pulpbot> https://pulp.plan.io/issues/6586
14:32:34 <bmbouter> #info bmbouter has joined triage
14:32:34 <bmbouter> !here
14:32:35 <pulpbot> bmbouter: bmbouter has joined triage
14:32:52 <dkliban> mikedep333: should we add to the sprint?
14:32:58 <fao89> #idea Proposed for #6586: accept and add to sprint
14:32:58 <fao89> !propose other accept and add to sprint
14:32:58 <pulpbot> fao89: Proposed for #6586: accept and add to sprint
14:33:09 <dkliban> +1
14:33:10 <mikedep333> I think so. It primarily affects CI, could affect users.
14:33:13 <daviddavis> +1
14:33:19 <fao89> #agreed accept and add to sprint
14:33:19 <fao89> !accept
14:33:19 <pulpbot> fao89: Current proposal accepted: accept and add to sprint
14:33:20 <pulpbot> fao89: 3 issues left to triage: 6583, 6581, 6569
14:33:20 <fao89> #topic https://pulp.plan.io/issues/6583
14:33:21 <pulpbot> RM 6583 - evgeni - NEW - coreapi is deprecated/unmaintained
14:33:22 <pulpbot> https://pulp.plan.io/issues/6583
14:33:31 <dkliban> accept and add to sprint
14:33:36 <fao89> #idea Proposed for #6583: accept and add to sprint
14:33:36 <fao89> !propose other accept and add to sprint
14:33:37 <pulpbot> fao89: Proposed for #6583: accept and add to sprint
14:33:37 <dkliban> i don't think we actually use this dep anymore
14:33:49 <dkliban> i'll comment on the issue
14:34:23 <x9c4> +1
14:34:31 <ttereshc> #info ttereshc has joined triage
14:34:31 <ttereshc> !here
14:34:31 <pulpbot> ttereshc: ttereshc has joined triage
14:34:42 <ttereshc> +1
14:34:47 <ipanova> #info ipanova has joined triage
14:34:47 <ipanova> !here
14:34:47 <pulpbot> ipanova: ipanova has joined triage
14:34:51 <fao89> dkliban, so should I accept and add to sprint or not?
14:35:03 <dkliban> #idea Proposed for #6583: accept and add to sprint
14:35:03 <dkliban> !propose other accept and add to sprint
14:35:03 <pulpbot> dkliban: Proposed for #6583: accept and add to sprint
14:35:04 <bmbouter> yeah I'm not familiar w/ this dep
14:35:13 <fao89> #agreed accept and add to sprint
14:35:13 <fao89> !accept
14:35:13 <pulpbot> fao89: Current proposal accepted: accept and add to sprint
14:35:14 <pulpbot> fao89: 2 issues left to triage: 6581, 6569
14:35:14 <fao89> #topic https://pulp.plan.io/issues/6581
14:35:15 <pulpbot> RM 6581 - iballou - NEW - Pulpcore delete_orphans took too long
14:35:16 <pulpbot> https://pulp.plan.io/issues/6581
14:35:31 <ipanova> #idea Proposed for #6581: accept and add to sprint
14:35:31 <ipanova> !propose other accept and add to sprint
14:35:31 <pulpbot> ipanova: Proposed for #6581: accept and add to sprint
14:35:39 <dkliban> +1
14:35:41 <ppicka> +1
14:35:41 <fao89> #agreed accept and add to sprint
14:35:41 <fao89> !accept
14:35:41 <pulpbot> fao89: Current proposal accepted: accept and add to sprint
14:35:42 <fao89> #topic https://pulp.plan.io/issues/6569
14:35:43 <pulpbot> fao89: 1 issues left to triage: 6569
14:35:44 <pulpbot> RM 6569 - lieter - NEW - Unable to add metadata_signing_service to existing repository
14:35:45 <pulpbot> https://pulp.plan.io/issues/6569
14:36:23 <fao89> #idea Proposed for #6569: Leave the issue as-is, accepting its current state.
14:36:23 <fao89> !propose accept
14:36:23 <pulpbot> fao89: Proposed for #6569: Leave the issue as-is, accepting its current state.
14:37:07 <ttereshc> it seems like a feature
14:37:18 <ttereshc> adding a signing service to the existing repo
14:37:20 <ttereshc> ?
14:37:27 <x9c4> No, i think its a bug
14:37:31 <bmbouter> well I believe the claim that it would work was implicit
14:37:33 <lieter> from the API description it shoudl work btw
14:37:39 <bmbouter> so if the claim is already there it's a bug
14:37:41 <ttereshc> ok
14:37:46 <x9c4> As the update task uses the wron serializer.
14:38:04 <dkliban> interesting
14:38:05 <x9c4> should be easy t fix at least.
14:38:08 <bmbouter> I'm in favor of accept and add to sprint
14:38:09 <dkliban> i think we should add to the sprint then
14:38:13 <ipanova> agreed
14:38:20 <fao89> #idea Proposed for #6569: accept and add to sprint
14:38:20 <fao89> !propose other accept and add to sprint
14:38:20 <pulpbot> fao89: Proposed for #6569: accept and add to sprint
14:38:24 <x9c4> +1
14:38:29 <fao89> #agreed accept and add to sprint
14:38:29 <fao89> !accept
14:38:29 <pulpbot> fao89: Current proposal accepted: accept and add to sprint
14:38:30 <pulpbot> fao89: No issues to triage.
14:38:38 <fao89> Open floor!
14:38:45 <ttereshc> fao89, a rule of thumb, say something other will be opposed to and that will make a conversation started :D
14:39:01 <dkliban> i want to go back to our discussion about write_only fields
14:39:12 <daviddavis> I could use reviews on https://github.com/pulp/pulp_file/pull/385 and https://github.com/pulp/pulp_file/pull/380
14:39:25 <dkliban> after i sent out the note to pulp-dev list i had some more discussion about write_only fields with x9c4
14:40:26 <dkliban> and the conlcusion i came to is that we can use write_only fields and generate new serializers for the POST operations as long as we don't change the name of the GET serializer
14:41:04 <dkliban> and if the name of that original serializer doesn't change, bindings users will be able to continue using that serializer, but they won't have access to any newly added write_only fields
14:41:13 <dkliban> so it will not be a breaking change for them
14:41:30 <dkliban> but if they want to use those newly added write_only fields for POST they will have to start using the new serializer
14:41:35 <daviddavis> what if the write_only field is required tho?
14:41:46 <dkliban> their code needs to be updated then
14:41:49 <dkliban> anyway
14:41:58 <daviddavis> that's true
14:42:04 <x9c4> Will they ever use the GET serializer explicitly?
14:42:17 <dkliban> x9c4: what do you mean?
14:42:58 <x9c4> When creating / updating a *thing* they need to create the write serializer.
14:43:18 <bmbouter> daviddavis: lgtm on both those Prs if you want to merge
14:43:23 <daviddavis> bmbouter++
14:43:23 <pulpbot> daviddavis: bmbouter's karma is now 256
14:43:31 <x9c4> But for getting it they only need the api object.
14:43:39 <dkliban> that's right
14:44:14 <x9c4> so not reanaming the write serializer seems like the thing to do.
14:44:33 <x9c4> because the read serializer will be instanciated by the nderlying code.
14:45:15 <dkliban> i see what you are saying
14:45:42 <bmbouter> if we use write_only fields will they be marked as write_only in the schema itself? or would the split serializer just show fields on the POST/PATCH and not the GET?
14:45:52 <daviddavis> the latter I think
14:45:53 <dkliban> the latter
14:45:56 <quba42> Spam issue that needs to be deleted: https://pulp.plan.io/issues/6576
14:47:34 <daviddavis> I'm +0 on keeping write_only fields and automatically splitting the serializers. I'd assume this would be documented for the plugin writers that this magic is happening.
14:47:37 <dkliban> so if the a user is using FileFileCOntent model to create File Content, they can continue using it. however, if they have code that does some kind of introspection on the model that it receives from a GET request, this will be a breaking change
14:49:03 <daviddavis> yea, any idea if katello ever does this?
14:49:13 <bmbouter> in this current working idea do plugin writers split the serializer or does the apigenerator auto-handle it?
14:49:16 <x9c4> What kind of introspection?
14:49:25 <fao89> I think we need to talk with katello team to get some feedback, depending the way the handle it, we document the change
14:49:34 <x9c4> like obj.__class__.__name__?
14:49:39 <dkliban> bmbouter: the current working idea is to have the openapi generator do the splittling
14:49:51 <dkliban> x9c4: of isinstance()
14:49:54 <dkliban> or
14:50:03 <daviddavis> what about type()?
14:50:07 <fao89> for me it would be like: try: Model except: Model + "Write"
14:50:09 <dkliban> yeah ... things like that
14:50:44 <dkliban> jsherrill: does katello code do some sort of type checking on Content models it recieves from the bindings?
14:51:09 <dkliban> i bet tehre is code that is conditional on types
14:51:31 <dkliban> and this will require refactoring for this change
14:51:56 <dkliban> however, even if plugin writers provide separate models, katello will need to refactor
14:52:21 <bmbouter> agreed, and also I think the requirements will require the serializers to be split to fix the bug which semver allows
14:52:29 <jsherrill> dkliban: can you give an example?
14:53:18 <jsherrill> oh like checking the model classes?
14:53:22 <daviddavis> yea
14:54:30 <jsherrill> i doubt we do
14:55:11 <jsherrill> is  the name going to change?
14:55:18 <fao89> example: for now FileFileRemote does not have username and password fields, as they are write_only. We are thinking about creating FileFileRemoteWrite that would have these fields
14:55:55 <dkliban> fao89: we don't be changing the Remote models
14:56:06 <dkliban> fao89: thos fields just need to hav ethe write_only removed
14:56:27 <fao89> so if you use a model from client, currently it does not provide write_only fields
14:56:32 <dkliban> but FileFileContent we want to split into FileFileContent and FileFileContentRead
14:56:52 <fao89> dkliban, it is the default example in my mind
14:57:49 <jsherrill> so the content units themselves changing i don't think would have any impact, we basically call .as_json in ruby to convert it to a hash as soon as we get it it looks like
14:58:00 <dkliban> jsherrill: i don't think you will be affected by the proposed change if you don't perform type checking on the object that you receive from the READ and LIST operations
14:58:08 <jsherrill> k
14:58:09 <bmbouter> fao89: actually can we use another example, my proposal for those fields is that they should be in the GET itself
14:58:28 <dkliban> yeah ... the example is most clear with FileFileContent
14:58:38 <bmbouter> but that's another convo altogether. dkliban agreed
14:59:54 <dkliban> so i would like to summarize this on the thread that i started on Friday
15:00:05 <dkliban> is everyone ok with that next step?
15:00:06 <bmbouter> that would be sweet
15:00:11 <fao89> jsherrill, does katello use some model from the client? Or it just uses the APIs?
15:01:11 <x9c4> dkliban, agreed.
15:01:49 <jsherrill> fao89: we use the models quite a bit, but more for remotes/repos/publications/distributions
15:02:01 <dkliban> none of those have write_only fields
15:03:04 <x9c4> But noone stops a plugin from adding some.
15:03:48 <dkliban> and with the current proposal this wouldn't really break anything
15:03:50 <jsherrill> and we can work through that, if it happens
15:04:04 <dkliban> except if you do some sort of type checking on what is returned on GET
15:04:11 <dkliban> jsherrill++
15:04:11 <pulpbot> dkliban: jsherrill's karma is now 44
15:04:19 <dkliban> thanks for sharing
15:04:37 <x9c4> jsherrill++
15:04:37 <pulpbot> x9c4: jsherrill's karma is now 45
15:04:39 <x9c4> dkliban++
15:04:39 <pulpbot> x9c4: dkliban's karma is now 459
15:05:12 <dalley> dkliban, re: your proposal, I'm worried that it will severely impact the complexity and speed of "what content does this repoversion contain" queries, which is already one of the most expensive queries we perform and probably the most important one
15:05:39 <ipanova> dkliban: daviddavis yes i know about that one, we use it in the migration plugin also, however i don't think extending it would look better than what i have in the PR. we cannot call cast on repo version because it is a not a master model and among the registered viewset we'll have RpmRepoVersionViewset and not RepoversionViewset https://github.com/pulp/pulpcore/blob/b93db9e9ff6140dc6cc6456d78049a28d678c573/pulpcore/app/util.py#L71 .
15:05:39 <ipanova> found in pulpcore a similar approach to what i have written  https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/repository.py#L843
15:06:11 <dkliban> is this still open floor
15:06:14 <dkliban> ?
15:06:19 <dkliban> or is that over?
15:06:34 <dkliban> cause i have a meeting to go to
15:07:20 <dkliban> dalley: i agree ... we will need to do some performance analysis
15:08:06 <daviddavis> ipanova: any idea if reverse() works? either way, your code is ok IMO
15:08:13 <dkliban> we should end open floor fao89
15:08:17 <ttereshc> +1
15:08:21 <fao89> #endmeeting
15:08:21 <fao89> !end