Repository navigation
Conversation
The installer workflow built an MSI and uploaded it without ever installing it, so nothing caught an install that registered no service, a PORT property that never reached the service, or a major upgrade that left the product in a broken state. Add three jobs to the installer workflow: a silent install that checks the service is registered, automatic and running, that the app answers on its port and that the log directory exists, followed by an uninstall that must leave no service behind; the same install driven with PORT=8123; and an in-place upgrade from the released build to one whose version was bumped. The upgrade build comes from build-backend, which now rebuilds the bundle at 9.9.9 via uv version so the MSI ProductVersion and the version the running app reports both move from the same source. Installing v1 on a custom port makes the port double as the upgrade marker, since a major upgrade that drops PORT falls back to the default and stops answering where the test looks.
bundle-executable-from-staged-static passed only --log-level=DEBUG, so a second build in the same checkout hit PyInstaller's 'output directory is not empty' prompt and aborted with no terminal to answer it. That surfaced when the installer upgrade test started rebuilding the bundle at a bumped version in a job that had already built it once. Move --noconfirm onto the shared pyinstaller invocation instead of adding it to the second call site, since no caller wants an interactive prompt and the next one would otherwise rediscover this.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
build-installer is called both by CI, which builds its own bundle, and by release, which passes the run id of an older CI run. The upgrade steps were unconditional, so a release reached back into that older run for the bumped bundle and built a 9.9.9 MSI it never ships. Once that bundle aged out, the release would have failed on a missing artifact. Gate the upgrade steps and all three test jobs on artifact-run-id being empty, so they run only in CI. The bumped bundle is then consumed within the run that produced it and drops to a one day retention.
PORT was declared with a fixed default and never persisted, so a major upgrade, which uninstalls the old product before installing the new one, brought the service back on the default port. An install on any other port silently moved after the first upgrade. Write PORT to HKLM under the product key and read it back with a RegistrySearch during AppSearch, which runs while the previous install's value is still present. The default now comes from a SetProperty that only fires when neither the command line nor the search supplied a value. Found by the upgrade-test CI job on its first run: it installed v1 on port 8123, upgraded, and the healthcheck on 8123 was refused.
The RegistrySearch was bound to PORT itself, so AppSearch overwrote whatever a caller passed as PORT= on the command line. An upgrade that deliberately moved the port would have been silently pinned to the previous one. Search into REMEMBERED_PORT instead and copy it into PORT only when PORT is empty, then apply the default only if PORT is still empty. Precedence is now command line, remembered value, default. The two SetProperty actions need explicit Action names because they share the PORT id.
…d task bundle-executable exists to hold one pyinstaller invocation whose arguments each caller supplies; folding --noconfirm into the shared command took that decision away from the call sites. Put it back on both callers instead, which fixes the same prompt-on-populated-dist failure without changing what the shared task is for.
A major upgrade recreates the service, and SERVICE_ACCOUNT fell back to its LocalSystem default, so the account dialog showed LocalSystem regardless of what the service was actually running under. An operator clicking through an upgrade moved the service off its account with nothing to indicate it had happened. Persist SERVICE_ACCOUNT and SERVICE_USER alongside PORT and read them back the same way, so the dialog opens on the existing account. SERVICE_PASSWORD is not persisted; storing it would leave it readable in the registry, so the operator retypes it and the dialog already refuses to continue until they do. This narrows the attended upgrade only. A silent upgrade still has no password to offer, so a service under a custom account cannot be recreated unattended.
The existing upgrade job only ever runs as LocalSystem, so it passes whether or not SERVICE_ACCOUNT and SERVICE_USER are remembered. Nothing covered the fix that persists them. Add a job that creates a throwaway local account, installs under it on a custom port, then upgrades supplying nothing but the password, so the account, the user name and the port all have to come back from the registry. It asserts the service still runs as that account afterwards, and that the password reached neither installer log, which is what Hidden on SERVICE_PASSWORD is for.
The rest restated the precedence comment above it and narrated an absence the three RegistryValue lines already show.
Both trailed off into restating what the surrounding step does.
Windows Installer writes the raw command line near the top of a /l*v log before any property masking applies, so a silent install passing SERVICE_PASSWORD as a property left the password in that log in clear text. Hidden was doing its job everywhere else, but it cannot reach the engine's own echo of the command line. Read the password from <PRODUCT>_INSTALL_PASSWORD when SERVICE_PASSWORD was not supplied, so a silent install has nothing secret on the command line. An explicit property still wins, and the UI path is unchanged. INSTALL.txt documents the environment variable as the way to do an unattended install under an account. The CI job now installs that way too, and asserts the password appears nowhere in either log while SERVICE_PASSWORD is still shown masked, so the masking going missing would fail the job rather than pass quietly.
…ounts The environment variable never reached the install: /qn skips InstallUISequence, so SetServicePasswordFromEnv only ran in the elevated service process, which does not have the operator's environment, and the deferred action received an empty password. Go back to the command-line property and note the logging caveat in INSTALL.txt instead. Also stop requiring a password when the account name ends with $. That suffix is how SCM is told the account is AD-managed, so a gMSA or dMSA legitimately arrives without one. A supplied password is still used either way, because $ is legal in an ordinary account name and discarding it would break such an install.
…ual dispatch, and correct the account docs The healthcheck probes fired a single request right after msiexec returned, but Start-Service reports RUNNING before uvicorn has bound the socket, so they raced the service and would flake. They now poll for up to two minutes. workflow_dispatch resolves to an earlier CI run whose upgrade bundle is kept for only a day, so the upgrade legs are skipped for that event rather than failing on an expired artifact. The ServiceAccountDlg Next button still demanded a password for every custom account, which made a group managed service account unreachable through the UI even though install-service.ps1 accepts one. The log warning in INSTALL.txt also contradicted the Hidden property flag: the value is masked unless the Windows Installer Debug policy is 7. INSTALL.txt now also states that the password is not remembered across an upgrade while the account and port are. Project names and manufacturers containing an apostrophe are escaped where they are interpolated into PowerShell string literals, and the redaction step exits early when the account step never set the password.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this change necessary?
We now build some installers. We should verify that they work as expected when installed, uninstalled and updated.
How does this change address the issue?
Adds tests in CI to do just that
What side effects does this change have?
N/A
How is this change tested?
Downstream
Other