15:05:51 <sdoran> #startmeeting Ansible Core Public IRC Meeting https://github.com/ansible/community/issues/543 15:05:51 <zodbot> Meeting started Thu Jul 2 15:05:51 2020 UTC. 15:05:51 <zodbot> This meeting is logged and archived in a public location. 15:05:51 <zodbot> The chair is sdoran. Information about MeetBot at http://wiki.debian.org/MeetBot. 15:05:51 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic. 15:05:51 <zodbot> The meeting name has been set to 'ansible_core_public_irc_meeting_https://github.com/ansible/community/issues/543' 15:06:03 <sdoran> #topic https://github.com/ansible/ansible/issues/70147 15:07:14 <sdoran> We had a lot of discussion around this and I think abadger1999 summed it up well here https://github.com/ansible/ansible/issues/70147#issuecomment-646712442 15:07:39 <felixfontein> ah right, it's meeting time! 15:09:14 <sivel> I still think that install should just ignore anything from site-packages, and install the deps it requires at install time 15:14:15 <maxamillion> .hello2 15:14:15 * maxamillion looks at zodbot 15:14:15 <zodbot> maxamillion: maxamillion 'Adam Miller' <maxamillion@gmail.com> 15:14:15 <gundalow> #chairs 15:14:15 <gundalow> #chair 15:14:15 <gundalow> shrugs 15:14:15 <sdoran> #chair maxamillion gundalow fel 15:14:15 <zodbot> Current chairs: fel gundalow maxamillion sdoran 15:14:15 <sdoran> #chair felixfontein 15:14:15 <zodbot> Current chairs: fel felixfontein gundalow maxamillion sdoran 15:14:15 <gundalow> sdoran: thanks for that summary 15:14:15 <maxamillion> @hello2 15:14:15 <maxamillion> I really thought it was . 15:14:15 <gundalow> I guess my question is what (subset) of this could/should/must be done for 2.10? 15:14:15 <shertel> o/ 15:14:15 <maxamillion> .hello2 15:14:15 <sdoran> #chair shertel 15:14:15 <zodbot> Current chairs: fel felixfontein gundalow maxamillion sdoran shertel 15:14:15 <maxamillion> oh wait, maybe that's fedbot 15:14:16 <sdoran> I think solving the issue for `collection list` is much easier than `install`. 15:14:16 <gundalow> maxamillion: .hello? 15:14:16 <gundalow> maxamillion: `.hello`? 15:14:16 <sdoran> Is anybody in there? 15:14:16 <gundalow> yup, I'm OK with `install` being out of scope 15:14:16 <maxamillion> gundalow: well .hello2 is supposed to do a FAS lookup and if your irc nick is in your FAS entry it auto-yanks the email address and stuff but maybe that's gone ... I actually don't know if FAS is still in play, I know it was planned to be decommissioned at some point 15:14:16 <gundalow> I'd personally like to see `collection list` working for 22.10 15:14:16 <sdoran> gundalow: Define "working" :) 15:14:16 <gundalow> I think collections are confusing enough for end users, if `collection list` doesn't work, then it's gona be really confusing 15:14:16 <sivel> the ACD bundled collections should only be used when the user isn't being explicit. Installing another collection should be explicit, and fully override site-packages 15:14:16 <gundalow> sdoran: something something list collections included in the `ansible` package 15:14:16 <sivel> So my recommendation, was that site-packages would only be applied in listing 15:14:16 <shertel> yeah, I agree with sivel 15:14:18 <zodbot> maxamillion: maxamillion 'Adam Miller' <maxamillion@gmail.com> 15:14:18 <sdoran> gundalow: <nod> That's a change in scope from design of the original feature. 15:14:18 <sdoran> But I think we can address that before release. 15:14:18 <shertel> I'm okay with applying it to verify too 15:14:18 <sivel> (also sorry, I lagged out, and may have lost some of the discussion) 15:14:19 <sivel> I'd be fine with verify too 15:14:19 <sivel> so list and verify, but not install (resolving deps) 15:14:37 <gundalow> lol 15:14:53 <maxamillion> heh 15:15:40 <resmo> zodbot got some AI 15:15:41 <sdoran> sivel: Does that me `install` should not look at `site-packages` at all? 15:15:49 <sivel> sdoran: correct 15:16:15 * resmo waves 15:16:25 <sdoran> Do we put `site-packages` at the bottom of the `COLLECTIONS_PATH`? 15:16:31 * sdoran waves at resmo 15:16:34 <sdoran> #chair resmo 15:16:34 <zodbot> Current chairs: fel felixfontein gundalow maxamillion resmo sdoran shertel 15:16:54 <shertel> otherwise, consider the scenario of trying to upgrade an individual collection that's also installed in ACD (since we reinstall to the current location with --force) 15:17:03 <gundalow> abadger1999: Just so you see this in scrollback 15:17:23 <shertel> you'd need to disable COLLECTIONS_SCAN_SYS_PATH to get around that 15:17:24 <sivel> sdoran: yes, site-packages go at the bottom 15:17:32 * abadger1999 looks in from docs ad hoc meeting 15:18:04 <sdoran> Just wondering what happens if `community.general` 0.0.8 is in `site-packages`, but I install `community.general` 1.0.0. 15:18:16 <gundalow> part of me wonders if `install` should error if someone attempts to overide site-packages. Just to avoid anyone doing anything funky 15:18:20 <gundalow> I need to drop off now 15:18:41 <abadger1999> So... install should also take site-packages into account 15:18:43 <sivel> sdoran: the explicitly installed version always wins 15:18:43 <sdoran> I would expect `install` to work fine (not complain about existing collection in `site-packages`) _and_ that the `1.0.0` version would be used when running playbooks by virtue of being higher in the stack. 15:18:47 <sdoran> Ok. 15:19:03 <sdoran> #unchair fel 15:19:03 <zodbot> Current chairs: felixfontein gundalow maxamillion resmo sdoran shertel 15:19:05 <shertel> sdoran: unless site-packages is provided in the collections path for installation, I don't think it should 15:19:07 <sdoran> #chair abadger1999 15:19:07 <zodbot> Current chairs: abadger1999 felixfontein gundalow maxamillion resmo sdoran shertel 15:19:08 <abadger1999> otherwise it gets confusing to the user why install and list are doing two separate things. 15:19:10 <felixfontein> sdoran: thanks :D 15:19:26 <sivel> abadger1999: I think at the moment, at least a few of us believe the opposite. 15:19:39 <sdoran> felixfontein: I didn't let my tab complete finish before hitting return. ð 15:19:43 <shertel> sdroan: +1 , that's how I'd expect it to work too 15:20:05 <sdoran> Yeah, that's what I'm trying to resolve. 15:20:14 <sdoran> Seems we have two different ideas of the behavior. 15:20:25 <sdoran> I can see both sides of the argument. 15:20:28 <abadger1999> @sivel I have only seen you disagree with that, actually... 15:20:40 <abadger1999> But the behaviour is nuanced. 15:20:43 <sivel> shertel: was agreeing with me 15:21:05 <sdoran> But if `install` errors when trying to install a newer version of a collection that's in `site-packages` that seems to break the idea of being able to update modules independently of `ansible-base`. 15:21:10 <abadger1999> shertel: has also not contradicted what I said, though ;-) 15:21:13 <shertel> yeah. I'm confused where the confusion is, actually. 15:21:15 <abadger1999> Sorry, 15:21:26 <abadger1999> sivel , shertel has also not contradicted[...] 15:21:39 <shertel> adadger1999: the current PR is not right because it applies to install, imo 15:21:57 <abadger1999> So, I think sdoran is accutately saying what install should do in that particular case. 15:22:00 <shertel> is that your understanding too? 15:22:06 <abadger1999> shertel: no... 15:22:24 <abadger1999> but there's problms with the PR due to how upgrade is handled. 15:22:36 <abadger1999> I think that requires chaqnges to how upgrade is handled. 15:22:38 <shertel> I may not have reviewed the PR carefully enough then 15:22:48 <abadger1999> rather htan disregarding site-packages i ninstall 15:22:55 <shertel> I don't think it should include install until --force/upgrade is resolved 15:23:03 <abadger1999> (because right now it doesn't do what sdoran outlined above and that's a problem) 15:23:16 <abadger1999> <nod> yeah... I think they're tied together. 15:23:42 <abadger1999> Are you okay with trying to outline how it should look once upgrade is resolved first, and then figureing out how to limit that until then? 15:23:44 <sivel> To be clear on my stance, `install` should completely ignore site-packages. 100% never consult it at all. 15:23:59 <sdoran> I thought `upgrade` was a separate feature request? Or you mean `install --force`? 15:24:00 <shertel> sivel: if the user adds site-packages to collections path? 15:24:07 <shertel> that's when it should, in my opinion 15:24:17 <felixfontein> shertel: I agree 15:24:23 <sivel> shertel: that is an unsupported configuration, and the user shoots themselves in the foot 15:24:32 <sdoran> So much crossing of the streams... 15:24:33 <shertel> sivel: agreed 15:24:43 <shertel> I don't agree with the auto-adding for install 15:24:52 <shertel> but I don't think we should necessarily error 15:25:02 <shertel> (was that what gundalow suggested?) 15:25:44 <sdoran> Well, that's suggested as a workaround in the topic issue. 15:25:47 <abadger1999> I think it should be: If the dependency/request is satisfied by something in site-packages, then it should not install. If the dependency or request is not satisfied because of version mismatch, then it should install to a directory that the user has access to write to. (cornercase I could go in either direction: if the request is not satisfied due to version but the user can write to site-packages) 15:25:54 <sdoran> In that case, it's just another path. 15:26:08 <sdoran> But I don't think we should be recommending that. We should fix the behavior. 15:26:53 <sdoran> Hmm, that is nuanced. 15:27:13 <sdoran> Ugh, deps. 15:27:19 <abadger1999> (I agree with shertel too, if site-packages is explicitly added to ANSIBLE_COLLECTIONS_PATHS, then it should just be a regular path) 15:27:34 <sdoran> ð 15:27:39 <abadger1999> Anyhow, that's what I think we should shoot for. 15:27:55 <abadger1999> But the upgrade problem prevents that from working correctly right now. 15:28:35 <abadger1999> because if you can't write to that location, then you get an error. 15:28:53 <sivel> Based on how the system works now, and where we are in the release, the only thing I would support for merge is a 100% ignoring of site-packages for install 15:29:34 <abadger1999> <nod> But do we agree that it should work the way I outline long term? 15:29:38 <sivel> If we can improve the install behavior in general... 15:29:47 <abadger1999> becaues that's important.... 15:29:52 <sivel> To properly support upgrades and such 15:30:00 <abadger1999> otherwise we probably need to do something different for list and verify as well. 15:30:04 <sivel> then I think I'd be ok with consulting it 15:30:47 <sivel> abadger1999: generally speaking, long term I think your proposal is probably do-able 15:31:05 <sivel> I think we need to sit down and really draft out how it should be working from a larger scale 15:31:07 <abadger1999> Cool. 15:31:18 <abadger1999> <nod> yeah, that's what I like to start with :-) 15:31:29 <abadger1999> Thus, why I want to agree on the way it should look long term. 15:31:30 <sdoran> That's probably the cleanest solution in the short term: `install` just creates all new collections in a user-writable path. 15:31:54 <sdoran> Effectively "cutting over" from using `site-packages`, even if there's a dep in `site-packages` it _could_ have used. 15:32:13 <abadger1999> Then I can go ahead and hack up what I have to do something that doesn't conflict with teh long term vision but also doesn't introduce new features in the beta period. 15:32:33 <abadger1999> sdoran: +1 I like that. 15:32:37 <sdoran> That might be slightly confusing to the user, but it's probably ok in the short term. 15:33:18 <abadger1999> short term it makes sense. (one thing to note.... this issue applies to more than just site-packages....) 15:33:27 <sdoran> "I had depA v0.0.8 already in the output of `collection list`. After installing a collection, that needed that, I now have two copies of depA v0.0.8". 15:34:07 <abadger1999> For instance, there's a /usr/share/ansible/collections directory in the ansible default config for ANSIBLE_COLLECTIONS_PATHS. That will suffer from the same upgrade problem. 15:34:14 <sdoran> We definitely need a design discussion before handling the longer term solution. 15:34:19 <abadger1999> So I think that should also use the first writable dir. 15:34:26 <abadger1999> Does that sound correct to you? 15:34:59 <abadger1999> (ie, this isn't a special case for site-packages... it's a special case for unwritable dirs) 15:36:11 <sdoran> That sounds correct. 15:36:18 <abadger1999> sdoran: Hmmm... actually.... if we do that.. is there a reason not to use site-packages in install for now? 15:36:25 <abadger1999> in the way that I said? 15:36:46 <abadger1999> because there will no longer be an error when you upgrade and the directory is unwritable. 15:37:02 <sdoran> I was also thinking about if you ran `sudo ansible-galaxy collection install`... 15:37:03 <abadger1999> shertel and sivel as well ^ 15:37:21 <abadger1999> sdoran: yeah. that's the thing I mentioned above in the long term I could go either way. 15:37:34 <sdoran> <nod> 15:37:36 <sivel> I'm still concerned with the scale of necessary changes. Anything that is being merged at this point should be limited in scope 15:37:48 <abadger1999> sdoran: i should be able to prevent that from working if we want to. 15:38:09 <abadger1999> sdoran: but I probably can't prevent installing into /usr/share/ansible from working. 15:38:29 <abadger1999> sivel: <nod> So are you voting against sdoran's short-term proposal? 15:39:22 <sivel> I'm not sure I am clear on what that short-term proposal is 15:39:33 <abadger1999> <@sdoran> That's probably the cleanest solution in the short term: `install` just creates all new collections in a user-writable path. 15:40:21 <sdoran> My thinking on `install` just does everything in `$first_writable_dir` and ignores `site-packages` is it would nudge people in the direction of using and installing collections separately from `ansible-base`. 15:40:55 <abadger1999> The proposal would be: short term. If the directory containing the collection to be upgraded is not writable by the user, the collection will be written into the first writable directory instead. 15:41:27 <shertel> I'm not sure how it's better than making `install` use the explicit paths (not auto-append yet for that) 15:41:29 <sdoran> So we distribute collections with `anible`, but as soon as you `collection install`, you'll get collections installed outside of `site-packages`. 15:41:45 <sivel> There are problems with that, in that to even install a different version of a collection that you have installed, requires `--force` 15:41:56 <sdoran> Even if they already existed in `site-packages`. 15:41:58 <sivel> I mean as is, we typically install only to the first dir from the config 15:42:00 <abadger1999> shertel: the difference is if the dependency (or fresh request) is already satisfied by what's in site-packages. 15:42:52 <sivel> if we consult site-packages and a user tries to install community.general, regardless of where we install, we have no real means to allow that without a scary `--force` 15:43:02 <abadger1999> sivel: <nod> which would make everything coherent for the end user. 15:43:20 <sdoran> Right. I think we should avoid that by just having `install` ignore `site-packages` entirely for the time being. 15:43:24 <abadger1999> sivel: That's the case for all upgrades right now, though, yes? 15:43:32 <sdoran> Until we can design and implement a more nuanced approach. 15:43:36 <sivel> sdoran: ok, that is the exact thing I have been saying 15:43:38 <abadger1999> Which is the problem that figuring out upgrade needs to solve. 15:43:42 <shertel> abadger1999: but you'll need to refactor to use a new path 15:43:49 <sivel> for 2.10 we completely ignore site-packages for install 15:43:50 <shertel> sdoran: +1 15:43:52 <sdoran> But we will need to document that behavior somewhere. 15:44:18 <abadger1999> shertel: Possibly? I'm not sure what refactor means. But yeah, maybe... 15:44:21 <shertel> Yeah, we should document it 15:44:25 <sivel> I'm not really engaging right now in a design discussion for the future. I'm basically talking about 2.10 15:44:26 <sdoran> Because I can see a detailed reader saying "I already have this dep in `site-packages`. Why did I get another copy is `$first_writeable_dir`?" 15:44:38 <sdoran> Same. 15:44:45 <sdoran> I want to get MVP done for 2.10. 15:45:05 <sdoran> Which seems to be that `list` and `verify` will look at `site-packages`, but not `install`. 15:45:11 <sivel> yes 15:45:32 <shertel> abadger1999: we reinstall to the same location, so when you brought it up a couple weeks ago I started refactoring to see what it would entail and it seemed messy. But there may be a much better way to do it 15:45:42 <sdoran> And we should _not_ tell anyone to add `site-packages` to `COLLECTION_INSTALL`. ð 15:45:48 <abadger1999> <nod> And upgrade will fail if there's something to upgrade in a non-writable dir that's not site-packages too. 15:46:09 <sdoran> Right. I'd say that's expected behavior, though. 15:46:11 <shertel> yep 15:46:59 <abadger1999> shertel: <nod> yeah... I know how to do it but I don't know how deeply it would have to be plumbed in. 15:47:11 <abadger1999> sdoran: is it? 15:47:28 <abadger1999> I don't think it should be expected long term either. 15:47:34 <abadger1999> But short term, sure. 15:47:37 <sdoran> So is the consensus that `list` and `verify` should examen `site-packages` but `install` ignores it completely? 15:47:52 <sdoran> Short term. 15:47:55 <shertel> +1, for the short term. Also +1 to the general long term plans. 15:47:59 <sdoran> For 2.11, we can look to improve this. 15:48:27 <sivel> sdoran: +1 15:48:47 <sdoran> abadger1999: I would expect it to fail if it cannot write to a directory. Is that what you were asking? 15:49:19 <sdoran> If the dep is in `/usr/share/ansible` and I can't write to it, that should fail. 15:49:34 <sdoran> Guess we need a `--nodeps` option. ðĪĶââïļ 15:49:39 <shertel> we have one 15:49:46 <sdoran> ð 15:49:49 <shertel> also a force flag for deps 15:50:04 <abadger1999> So documentation we need: (1) site-packages is ignored for install [no workaround] (2) if a package is in an unwritable dir (/usr/share/ansible/collections), upgrades will fail [workaround, use env vars to remove that path from ANSIBLE_COLLECTIONS_PATHS while you run the install. but then that will also ignore things in that path which would have been okay.) 15:50:17 <sdoran> Making package managers is fun, they said. ðĪŠ 15:50:21 <abadger1999> sdoran: I would expect it to write to the first writable dir instead. 15:50:38 <sdoran> abadger1999: Oh, I see. I agree. 15:50:38 <abadger1999> possibly with the use of --force. 15:51:04 <abadger1999> <nod> 15:51:05 <sdoran> I thought you meant if it wasn't able to write to _any_ dirs. 15:51:09 <sdoran> Sorry. 15:51:11 <abadger1999> Heh :-) 15:51:21 <abadger1999> Yeah, I would definitely expect that to fail too :-) 15:51:57 <abadger1999> I'll take care of the code needed to implement the short term and open a ticket for someone else to deal with the long term. 15:52:05 <sdoran> Though if the order is `/usr/share/ansible:~/.ansible/`, and it puts the newer dep in `~/.ansible`, it won't get used during runtime. 15:52:09 <abadger1999> Who wants to take the documentation issues? 15:52:18 <sdoran> But that's a topic for the design session. 15:52:28 <abadger1999> sdoran: <nod> Yeah. I thought of that too and figured there should be a check about the order. 15:52:36 <sdoran> I'll add it here in a moment and then put a larger summary on the topic issue. 15:53:04 <abadger1999> if FIRST_WRITABLE.index < CURRENTLY_INSTALLED.index: proceed ; else: error() 15:54:08 <abadger1999> The code to implement the short term will be pretty invasive, btw. 15:54:21 <abadger1999> But hopefully it won't be invasive in the same places that you're worried about. 15:54:53 <sdoran> #agreed `ansible-galaxy collection list` and `verify` should examine `site-packages`, but `install` should ignore it completely and install the collection plus any deps to the first writeable dir in `COLLECTIONS_PATHS`. 15:55:46 <sdoran> Hmm. I guess we'll see what the code change looks like. 15:56:29 <sdoran> abadger1999: Is this going to be done as part of https://github.com/ansible/ansible/pull/70173 ? 15:56:54 <sdoran> #action abadger1999 to implement agreed upon solution 15:57:10 <sivel> I think as it is, we only ever try to write to the first configured path, otherwise any other paths must be explicitly provided with `-p` 15:57:13 <abadger1999> sdoran: I was going to but if you want me to leave that as a sample for the long term solution I can do that instead. 15:57:35 <abadger1999> *I can open a new PR instead. 15:57:53 <sdoran> abadger1999: Whatever works for you. I just wanted to know what to watch/review. 15:59:32 <sdoran> #info Future design meeting to be held about how to solve this longer term 15:59:35 <sdoran> Any other thoughts on this? We're about out of time. 16:00:22 <sdoran> Thank you everyone for attending and helping work through this issue. 16:00:24 <sdoran> #endmeeting