Serverless: Add separate command for completion install/uninstall (was: npm install mucks with preferences silently)

[DavidJFelix]

I've got my home directory in git so I noticed something that I perceived to be wildly bad manners. When I npm install -g serverless it makes global changes to my .zshrc without asking. My rc file is configured for multiple machines and tailored to my needs, as I think it is for many people -- having things haphazardly add themselves to it without my control feels invasive, IMHO*

The culprit lines of code that caused this are:
https://github.com/serverless/serverless/blob/66f019b1f36af41dd067ac598244e97f98e1ead8/scripts/postinstall.js#L30-L31

The use of tabtab's --auto flag is what I think is damaging. In auto mode, tabtab doesn't place a standalone file in the completions folder for bash/zsh, it just haphazardly dumps code into the rc file.

Maybe I'm being overly sensitive about packages changing files outside of their scope in stateful manners and convenience is king. I just wanted to start a discussion about this to see if this was a valid concern or if I should continue "being weird" and write protecting my zshrc so that npm install doesn't mess with it.

[pmuens]

Thanks for opening @DavidJFelix 👍

Yes, that's a good point and you're right that tabtab adds this config to the corresponding .<shell>rc file. We've discussed this in the past and came up with the conclusion that we should auto-add this so that everyone gets autocompletion out of the box without getting through a separate config step.

However maybe a prompt would be better here. Not sure how this affects the onboarding process when installing Serverless 🤔.

/cc @eahefnawy @brianneisler

[DavidJFelix]

If you use npm install -g serverless and it prompts you, this should be a reasonable interaction, but if you just npm install for some package.json in a project, prompting could get messy.

Personally, I think that if people aren't installing it globally, the completions don't make sense. What happens when you have 2 local packages which use different versions of serverless and the completions diverged between those versions?

I'm not really sure the best way to approach that determination, however.

[brettneese]

It is possible to check whether a module is install locally or globally: https://github.com/sindresorhus/is-installed-globally

(I'm not sure we need/want a dependency on that module, just that it shows it is possible. I'd have to dive a little bit deeper into making sure this is a reliable way of doing that.)

Autocompletions already happen in scripts/postinstall.js, so making sure we're in a global install and adding a confirmation prompt would be pretty simple. Plus, you could do what Brew does and output the thing that gets appended to the shell config so if someone is using multiple shells (eww) or has write-protected their or done any other weird magic they can just manually copypasta.

Local installs definitely shouldn't touch anything outside their scope.

Can you install packages using a package.json globally? Shell control could be a concern if you're npm installing multiple packages globally -- and ideally I don't want to have to manually confirm things that get npm installed from a package.json in environments without shell access (ie in CI) -- though I think it still dumps you into a prompt since it's the postinstall.js that has control at that point.

[DavidJFelix]

I think you can npm install -g with no packages, installing a full package.json globally. I'm not sure why you would.

That package is extremely small... 4 LOC. Either include it or duplicate it if it works. I think it's reasonable to assume the package will stay updated given the author, FWIW.

[kvadevack]

Maybe I'm being overly sensitive about packages changing files outside of their scope

I think many would consider it bad form, actually. @pmuens , have you guys considered offering completions as a separate node module, e.g. serverless-completions?

[zenlor]

User friendliness is important. But it doesn't mean you need to spoonfeed your users. This is a tool for developers, I don't think we need to have completion out of the box for a program without explicitly asking for it.

This tabtab thing is messy, it would add lines to my dotfiles either if I want or not. The best way to handle this is to explicitly ask your users to copy-paste completion snippets.

In my case, for example, I would add it to my ~/.zsh.local not in my ~/.zshrc.

Please don't mess with user's own files without the user's input. It's a bad user experience.

[bpholt]

This is causing issues on our build servers because the tab completion install points to a temporary workspace that is deleted after the build. Subsequent builds fail because the [ -f path ] && . path added to .bashrc exits with a non-zero exit code, which the build server interprets as an overall failure. (The build user's .bashrc is being sourced to allow builds to use nvm.)

What needs to be done to make progress on this issue? It seems to be stagnant since August 2017.

[cornfeedhobo]

@pmuens Yes, please prompt or add a flag or something, but not be the default strategy. This is incredibly messy and breaks so many expectations.

[HyperBrain]

@cornfeedhobo I agree. Currently the tabtab installation is located in a package script hook. It might be better to move it to a completely separate serverless command like serverless install completion and serverless install completion --remove to let users explicitly add or remove the completion.
@horike37 What is your opinion on that?

[horike37]

@HyperBrain

Currently the tabtab installation is located in a package script hook. It might be better to move it to a >completely separate serverless command like serverless install completion and serverless install >completion --remove to let users explicitly add or remove the completion.

I agree. Sounds good solution.
Actually, as far as I've read this issue, many people against auto-add this completion so there is no reason for me to against the solution.

[indieisaconcept]

Also hit this issue. When my project was installed for the first time on our build agents it subsequently broke builds for all other projects.

Definitely in favour completions being installed either via a seperate module or initiated via a command.

[HyperBrain]

Ok, then I'll change the title for this issue to reflect it as the feature task to implement the completion installation. However we should now exactly specify _how_ it should be done (requirements) or is my proposal from above ok for everyone?

[cornfeedhobo]

@HyperBrain thanks! really looking forward to this change. Combined with nodenv it's causing some really bad confusion with profiles.

[axelson]

This is also breaking installation of serverless with the nodejs asdf version manager. See https://github.com/asdf-vm/asdf-nodejs/issues/73

[cornfeedhobo]

@HyperBrain I guess I'd decide that based on how you generate completion. If it's easy to break out into a package, instead of a subcommand, I get the sense that is generally more preferred.

[HyperBrain]

@cornfeedhobo Currently it is completely handled in the package scripts, so moving these to a separate repo would be much less efforts than implementing an additional command. Nice thought 👍

[HyperBrain]

@pmuens @horike37 What are your thoughts of just extracting the tabtab package and the install scripts to a separate repo @serverless and publish that as serverless-completions to npmjs?

[c0d3d]

Is someone currently working on this? I'd love to see this resolved.

[ahawkins]

Please don't mess with user's own files without the user's input. It's a bad user experience.

I agree. It's safe it assume that _developers_ installing and building applications are competent enough to manage completions. I keep my dotfiles under SCM, now I have a dirty git dir. That's definitely unexpected.

You can short circuit this by adding the installation markers to your config. Here's an example from my dotfiles: ahawkins/dotfiles@bee5e3c.

[axelson]

Can we please get the "needs-feedback" label removed from this issue? I don't think that any more feedback is needed at this point.

[axelson]

FYI, just created a PR that addresses this: https://github.com/serverless/serverless/pull/5706

[jeffbski]

I ran into issue with tabtab when trying to do npm rebuild in docker environment for lambda. Fails because it has to rebuild spawn-sync and can't chmod on directory. spawn-sync is not needed in node 8+ so newer version of tabtab help, but I would rather this be totally optional. Don't install tabtab and autocompletion unless user installs plugin or issues a command.

[jeffbski]

I created a PR which simply strips out autocomplete (and tabtab) #5875

It does not deal with making it a plugin or providing a command to install.

[pmuens]

Since there were quite a few attempts to fix this issue in the past I'd like to discuss potential solutions here before we move forward.

Generally speaking autocompletion is a feature most of the community members benefit from. It's especially useful for newcomers and non-power users who are unfamiliar with the feature-set Serverless offers.

On the other hand we have to admit that given tab-tabs implementation of messing around with the users shell profile is quite unfortunate. This is especially true for power-users who heavily customize their profiles and want to be in control with this.

Given those two conflicting opinions about auto-completion we propose that we make auto-complete opt-out via a command (something like serverless config --autocomplete uninstall). The LOC tab-tab adds to the users profile are deterministic so it should be possible for use to undo this (maybe tab-tab even supports the removal of such additions out of the box).

This is the best compromise between the user experience for day-to-day users who heavily rely on auto-completion and the desire to be in control of the shell profile setup.

[koterpillar]

Will opt-out be necessary for each project where serverless is installed? I'd like this functionality to be disabled globally, perhaps creating ~/.serverless-no-autocomplete is a better option?

[jeffbski]

Well maybe others have had better luck with serverless autocomplete, but personally I have never seen it work. Maybe I am doing something wrong, I don't know. In general on my mac, completions for other things like bash and git work great, but with serverless, tab-tab'ing doesn't seem to help me complete commands or options, so I have never seen the benefit. I have tried using the full serverless name rather than the shortcuts (sls, slss) and that makes no difference, tab complete always just gives me file names not serverless commands/options.

Unfortunately even disabling the feature does not solve my desire to remove the spawn-sync dependency (which requires a machine specific build). The only way to keep tab-tab and remove spawn-sync dependency would be to up severless requirement to use Node 8+ and then use a newer version of tab-tab which doesn't need spawn-sync. That would meet my desire if Node 8 is a reasonable minimum for serverless these days.

Are we really sure that most community members actually benefit from this feature such that it justifies keeping it in core and not just something that can be added with a plugin? From what I can tell it is only documented in a blog article for 1.15, I don't remember coming across it in the main serverless docs and searching google for it didn't turn up any other hits on serverless.com.

I could see it being somewhat helpful for autocompleting your function names (things that actually vary and you can forget what you called something), though I don't see that capability even being mentioned in the article, only commands and options, so I don't expect that it even does that. I have never saw the need to autocomplete standard commands or options since they are short enough and intuitive. If I ever forget, I simply type sls to see the help page to refresh my memory and mostly just while I am initially learning serverless. So assuming that it actually does work on some people's machines, I still question how many people know about it and how heavily the community relies on this feature.

sls invoke -s dev is pretty easy to type already. And I believe you actually have to type the longer command serverless (or enough to get a unique match in your bash, 4-5 chars) to even get completion in the first place for those machines where it works.

Maybe others have better luck with it working and love this feature, but I have to wonder. And even so I would still find it reasonable to just make it an optional plugin so we could eliminate the need for these dependencies requiring machine specific building. Just my opinions, your mileage may vary.

[cornfeedhobo]

@pmuens Opt-out design for something that modifies personal files is bad. Further, making it a sub-command that must be run after the modification has already been made, requiring two automated edits? Please _no_.

I also think the assumed added value is just incorrect. Auto-complete has never worked for me, and it sounds like this is the case for others. Also, this tool is used in CI/CD, where nothing like tab-tab should be being installed in the first place, most certainly not requiring a follow up "uninstall".

Break out auto-completion into a different package if you must rely on tab-tab and editing of personal shell configurations. Please re-open #5875 and get it merged (or @dschep, please weigh in there).

[DanielSeehausen]

another vote here to remove any editing of rc/profile without prompt

[cornfeedhobo]

It's sad that people are resorting to stuff like this still
https://github.com/ahawkins/dotfiles/commit/bee5e3cfefd5801feebd21467fee6cda31270162

[ahawkins]

I agree. I don’t understand why modifying a users $HOME files is the default behavior.

// Adam

On Jul 3, 2019, at 17:30, cornfeedhobo [e-mail] wrote:

It's sad that people are resorting to stuff like this still
ahawkins/dotfiles@bee5e3c

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[c0d3d]

Why isn't this fixed yet? Super irritating ...

[c0d3d]

I think #6337 is a reasonable solution for the time being. It seems no one is going to make the installer plugin, and I think it is better to fix this in the interim.

[dead10ck]

Why is there such a delay on this issue? This has been open for nearly two years. Opt-out of a behavior that modifies user's home files without their permission is unacceptable. If there is still disagreement over how to implement the installation, then you should remove it entirely until a decision is made. Editing a user's home files without their permission borders on malicious.

[c0d3d]

Not only is it editing user's home files, but it actually adds code that runs when the shell starts.

[cornfeedhobo]

@dead10ck Probably someone needs to write a smearing blog post, on medium of course, and then maybe they will understand that this is a serious infraction against existing standards. 2 years is a very long time, but you can also look back at the issues and PRs to see that they have continuously closed anything related to this.

@pmuens I can't stress this last point enough: the tab-tab completion has never worked for me. ever. not on suse, ubuntu, nix, or macos. And it regularly broke shells for my devs running zsh. I really see no point in perpetuating this pattern.

[c0d3d]

FWIW, tabtab has also never worked for me

[DavidJFelix]

Hi. I was going to wait until August 10th to send the above PR, but I keep getting pinged on this issue, so I decided to just do it.

[pmuens]

Thanks again for everyone who chimed in on this (controversial) Serverless Framework feature.

We hear you and we understand the pain that this caused in the past. I just wanted to give a quick update here and inform everyone that we're currently working on an implementation where the autocomplete setup is part of the interactive Serverless CLI experience we've just introduced lately. This way you're asked if you want to setup autocomplete on your machine (rather than updating your shell config automatically).

Thanks again for everyone who opened up PRs to fix this in the past. It means a lot to us to see people taking the initiative to question and improve the status quo. After all it's the community which makes this project so powerful and joyful to use.

Also /ccing @medikoo @billfine and @ganeshvlrk here

[with-heart]

Hi @pmuens, thanks for acknowledging the concerns around this! I see that this was added to 1.50.0 and has since been pushed back to 1.53.0. Is there anything the community could assist with to help this change be released sooner rather than later?

[medikoo]

@with-heart we'll do our best to push it with 1.53.0.

[with-heart]

@medikoo Thank you! ❤️