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: interactive postinst script breaks install for non-interactive environments #227837

Merged
merged 8 commits into from
Oct 31, 2024

Conversation

rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Sep 6, 2024

With the latest Stable release, first-time Debian package users have to confirm whether they want to add the Microsoft repository or not.

This PR adds more checks so that users in non-interactive or CI environments could opt in or opt out of installing the repository by explicitly setting the debconf value before starting the install. Machines without debconf are automatically opted in to installing the repository to match with the previous behaviour.

Closes #22145

@rzhao271 rzhao271 self-assigned this Sep 6, 2024
@rzhao271 rzhao271 modified the milestone: September 2024 Sep 6, 2024
@rzhao271 rzhao271 requested review from Tyriar September 6, 2024 21:24
@rzhao271 rzhao271 marked this pull request as ready for review September 6, 2024 21:28
Comment on lines 45 to 48
# Determine whether to write the Microsoft repository source list
# 0 means do not write
# 1 means ask and then write if Yes
# 2 means write without asking
Copy link
Member

Choose a reason for hiding this comment

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

So this would make the default behavior 0 where we no longer write? I think this would break our update story we've had forever and users would get stuck on older versions without realizing it. Perhaps the default should be determined on whether the terminal is interactive?

  • Is CODE_INSTALL_WITH_REPO set? Use that
  • Is interactive? CODE_INSTALL_WITH_REPO = ask
  • Is not interactive? CODE_INSTALL_WITH_REPO = write (old behavior)

The environment variable would give users control when it's important, we keep the old behavior when not interactive (automatically install repo for updates) and for interactive we ask users explicitly.

Then we can add to docs how CODE_INSTALL_WITH_REPO works and that you can run it like this:

CODE_INSTALL_WITH_REPO=0 ...

Maybe better to use strings instead of numbers so it's more obvious and you don't need to consult docs?

CODE_INSTALL_WITH_REPO="no" ...
CODE_INSTALL_WITH_REPO="ask" ...
CODE_INSTALL_WITH_REPO="write" ...

Also if CODE_INSTALL_WITH_REPO has an unexpected value we should probably warn and proceed with the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two variables. I have changed WRITE_SOURCE, the internal variable, to use 'yes', 'ask', or 'no'.
Meanwhile, the if conditions still only check if CODE_INSTALL_WITH_REPO = true or if CODE_INSTALL_WITH_REPO = false.
I also added a new condition for non-interactive terminals later in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the environment variable to match with the docs, which currently only mention debconf.

resources/linux/debian/postinst.template Outdated Show resolved Hide resolved
resources/linux/debian/postinst.template Outdated Show resolved Hide resolved
@rzhao271 rzhao271 modified the milestones: September 2024, October 2024 Sep 20, 2024
@rzhao271 rzhao271 force-pushed the rzhao271/prior-reptile branch from 8a9fc71 to 74e6923 Compare September 27, 2024 22:00
@rzhao271 rzhao271 requested a review from Tyriar September 27, 2024 22:09
@rzhao271
Copy link
Contributor Author

@rzhao271 rzhao271 changed the title fix: user must use debconf to configure install fix: interactive postinst script breaks install for non-interactive environments Sep 27, 2024
Tyriar
Tyriar previously approved these changes Oct 4, 2024
@rzhao271
Copy link
Contributor Author

rzhao271 commented Oct 4, 2024

I realized I forgot to verify the build.
Verification for the previous build failed.
New verification build: https://dev.azure.com/monacotools/Monaco/_build/results?buildId=297436&view=results

@rzhao271 rzhao271 force-pushed the rzhao271/prior-reptile branch from 2877a96 to b1b71c3 Compare October 30, 2024 18:23
@rzhao271
Copy link
Contributor Author

@rzhao271 rzhao271 requested a review from Tyriar October 30, 2024 21:51
@rzhao271
Copy link
Contributor Author

Verification build LGTM.

@rzhao271 rzhao271 merged commit 49bf27e into main Oct 31, 2024
21 checks passed
@rzhao271 rzhao271 deleted the rzhao271/prior-reptile branch October 31, 2024 14:50
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Dec 15, 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.

Allow users to opt-out of apt repository install
2 participants