Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: only add apt sources for users that want them (#22145) #221285

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

m-byte
Copy link
Contributor

@m-byte m-byte commented Jul 9, 2024

Currently, vscode.list is added to /etc/apt/sources.d/ whenever it doesn't exist. With this patch, the new behaviour is as follows:

  1. If the user already has the Microsoft source file installed for apt, sources won't be added
  2. If the user sets debconf-set-selections to set code/add-microsoft-repo to false, the sources won't be added
  3. If sources might be added, the script checks whether it makes sense to (over)write them (i.e. the file doesn't exist, is old or has been disabled during some OS upgrade)
  4. If is makes sense and the code/add-microsoft-repo is not set, the user is asked to cofirm, whether they actually want Microsoft sources to be installed (setting code/add-microsoft-repo to the selected value)
  5. Only if it makes sense and the user agrees, the sources are installed

This change will mostly affect new users or those reinstalling vscode. Personally, I feel like the whole approach of automatically adding the repo is very invasive, and would prefer that it not be done at all. However, with this change, it is up to the user whether they want it to be installed or not. Since the default is set to true, users that install vscode in noninteractive environments get the current behavior. Existing users will either be asked once or never (depending on whther step 4 above gets triggered). New users will be asked unless they make a decision ahead of time using debconf-set-selections.

With this, #22145 and duplicates should be sufficiently addressed.

@m-byte
Copy link
Contributor Author

m-byte commented Jul 9, 2024

@microsoft-github-policy-service agree

@m-byte m-byte changed the title Fix: only add apt sources for users that want it (#22145) Fix: only add apt sources for users that want them (#22145) Jul 9, 2024
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rzhao271 can you take this one? We need to be super careful of any changes here as a misstep could mean someone is stuck unknowingly on an old version.

@Tyriar Tyriar assigned rzhao271 and unassigned Tyriar Jul 9, 2024
@m-byte
Copy link
Contributor Author

m-byte commented Jul 31, 2024

@Tyriar @rzhao271 it would be great if you could look into this, as the current behavior is quite annoying on Ubuntu 24.04

resources/linux/debian/postinst.template Outdated Show resolved Hide resolved
resources/linux/debian/templates.template Outdated Show resolved Hide resolved
resources/linux/debian/templates.template Outdated Show resolved Hide resolved
@rzhao271 rzhao271 added this to the August 2024 milestone Jul 31, 2024
build/gulpfile.vscode.linux.js Outdated Show resolved Hide resolved
resources/linux/debian/postinst.template Outdated Show resolved Hide resolved
resources/linux/debian/templates.template Outdated Show resolved Hide resolved
resources/linux/debian/postrm.template Outdated Show resolved Hide resolved
@m-byte m-byte force-pushed the fix_apt_sources branch 3 times, most recently from 96ed45a to 5736c7a Compare August 1, 2024 06:04
@m-byte m-byte requested a review from rzhao271 August 1, 2024 12:29
resources/linux/debian/postinst.template Outdated Show resolved Hide resolved
resources/linux/debian/postinst.template Outdated Show resolved Hide resolved
resources/linux/debian/postinst.template Show resolved Hide resolved
Currently, vscode.list is added to /etc/apt/sources.d/ whenever it
doesn't exist. With this patch, the new behaviour is as follows:

1. If the user already has the Microsoft source file installed for apt,
   sources won't be added
2. If the user sets debconf-set-selections to set code/add-microsoft-repo
   to false, the sources won't be added
3. If sources might be added, the script checks whether it makes sense to
   (over)write them (i.e. the file doesn't exist, is old or has been disabled
   during some OS upgrade)
4. If is makes sense and the code/add-microsoft-repo is not set, the user is
   asked to cofirm, whether they actually want Microsoft sources to be
   installed (setting code/add-microsoft-repo to the selected value)
5. Only if it makes sense and the user agrees, the sources are installed

This change will mostly affect new users or those reinstalling vscode.
Personally, I feel like the whole approach of automatically adding the
repo is very invasive, and would prefer that it not be done at all.
However, with this change, it is up to the user whether they want it
to be installed or not. Since the default is set to true, users that
install vscode in noninteractive environments get the current behavior.
Existing users will either be asked once or never (depending on whther
step 4 above gets triggered). New users will be asked unless they make
a decision ahead of time using debconf-set-selections.

With this, microsoft#22145 and duplicates should be sufficiently addressed.

Signed-off-by: Matthias Breithaupt <m.breithaupt@vogl-electronic.com>
@m-byte m-byte requested a review from rzhao271 August 6, 2024 08:23
Copy link
Contributor

@rzhao271 rzhao271 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@rzhao271 rzhao271 requested a review from Tyriar August 6, 2024 16:37
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rzhao271 let's make sure this gets verified and documented at the end of the month

@rzhao271 rzhao271 closed this Aug 6, 2024
@rzhao271 rzhao271 reopened this Aug 6, 2024
@rzhao271 rzhao271 enabled auto-merge (squash) August 6, 2024 21:42
@rzhao271 rzhao271 merged commit cd340e6 into microsoft:main Aug 6, 2024
6 checks passed
@m-byte m-byte deleted the fix_apt_sources branch August 26, 2024 14:16
@andreineculau
Copy link

andreineculau commented Sep 6, 2024

This PR has not updated the installation docs https://github.com/microsoft/vscode-docs/blob/main/docs/setup/linux.md

but in its current form, it introduced a breaking changed as unattended installations (e.g. CI) will now block and ask for user input

#14 783.8 Setting up code (1.93.0-1725459079) ...
#14 783.8 update-alternatives: using /usr/bin/code to provide /usr/bin/editor (editor) in auto mode
#14 786.7 debconf: unable to initialize frontend: Dialog
#14 786.7 debconf: (Dialog frontend will not work on a dumb terminal, an emacs shell buffer, or without a controlling terminal.)
#14 786.7 debconf: falling back to frontend: Readline
#14 790.4 Configuring code
#14 790.4 ----------------
#14 790.4 
#14 790.4 The installer would like to add the Microsoft repository and signing key to
#14 790.4 update VS Code through apt.

What gives?

Hotfix: run echo "code code/add-microsoft-repo boolean true" | sudo debconf-set-selections before installing vscode.deb

@Tyriar
Copy link
Member

Tyriar commented Sep 6, 2024

@rzhao271 we should ensure that unattended installs work, either by explicit flag or the default. This was why the original proposal was an env variable; so those in the know could disable it easily but most users would get the easy "just works" setup.

@rzhao271
Copy link
Contributor

rzhao271 commented Sep 6, 2024

@andreineculau I mentioned the change in the release notes but forgot to update the docs.
I have created a new PR at #227837 that allows a user to set an env var rather than go through the interactive debconf prompt. Let me know what you think.

@andreineculau
Copy link

andreineculau commented Sep 6, 2024

@rzhao271 I don't see relevant info in the release, but regardless - I think it's the docs that should be tip-top.

As for using code/add-microsoft-repo or an env variable - I don't see that it matters. It may even be more idiomatic to leave it as is. Just 2c

Appreciate it! 🙇

@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Sep 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants