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