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