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

feat: unmanaged installs #1410

Merged
merged 2 commits into from
Sep 17, 2024
Merged

feat: unmanaged installs #1410

merged 2 commits into from
Sep 17, 2024

Conversation

mistydemeo
Copy link
Contributor

@mistydemeo mistydemeo commented Sep 11, 2024

The new "unmanaged" installation mode allows for something close to "untar and move on" installations. It disables modifying the PATH and creation of environment files, and ensures all files are installed into a single flat directory no matter what other configuration is in place. It also disables installing a standalone updater and the creation of an install receipt.

The new "don't install an updater/receipt" behaviour is also controllable behind a new public environment variable, $APPNAME_DISABLE_UPDATE.

Fixes #1400.

@ashleygwilliams
Copy link
Member

q: does this still attempt receipt creation? i think it'd be best to just not try at all

@mistydemeo
Copy link
Contributor Author

The issue sort of identified a conflict between the two user groups this attempts to serve - one would care about receipts, the other is neutral on them. I tried to split the difference by attempting receipt creation, but treating receipt creation failure as harmless.

@ashleygwilliams
Copy link
Member

hrm, i think splitting the difference here is maybe the less good option and we should consider 2 modes

@ashleygwilliams
Copy link
Member

part of the reason i say this is that an unmanaged install is desirable for ephemeral environments which are usually automated- receipt creation is an i/o operation and while not particularly expensive it's not free- this feature in my mind is specifically for the "conductor" persona and they are the type that care about milliseconds

@mistydemeo
Copy link
Contributor Author

mistydemeo commented Sep 12, 2024

Based on our previous discussion, I've made a few changes:

  • We no longer split the difference between two groups of users; this is now completely focused on automated/CI usecases, and the updater/receipt is unconditionally disabled.
  • The updater disabling functionality can be enabled separately with a new environment variable.
  • The environment variables have been moved into a new grouping in the manifest to make reasoning about them as a group easier:
-      "install_dir_env_var": "CARGO_DIST_SCHEMA_INSTALL_DIR",
+      "environment_variables": {
+        "install_dir_env_var": "CARGO_DIST_SCHEMA_INSTALL_DIR",
+        "unmanaged_dir_env_var": "CARGO_DIST_SCHEMA_UNMANAGED_INSTALL",
+        "disable_update_env_var": "CARGO_DIST_SCHEMA_DISABLE_UPDATE"
+      },

@ashleygwilliams
Copy link
Member

we could probably name that env for brevity's sake

@mistydemeo
Copy link
Contributor Author

Sounds good, done.

-      "environment_variables": {
+      "env": {

cargo-dist/templates/installer/installer.ps1.j2 Outdated Show resolved Hide resolved
let env_app_name = name.to_ascii_uppercase().replace('-', "_");
let install_dir_env_var = format!("{env_app_name}_INSTALL_DIR");
let unmanaged_dir_env_var = format!("{env_app_name}_UNMANAGED_INSTALL");
let disable_update_env_var = format!("{env_app_name}_DISABLE_UPDATE");
Copy link
Contributor

Choose a reason for hiding this comment

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

not DISABLE_UPDATER?

Copy link
Member

Choose a reason for hiding this comment

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

my thought here was that the user cares about the "update" behavior. there is not always an "updater" so "update" focused on the behavior the user understands, not the impl

cargo-dist-schema/src/lib.rs Show resolved Hide resolved
let install_dir_env_var = format!("{env_app_name}_INSTALL_DIR");
let unmanaged_dir_env_var = format!("{env_app_name}_UNMANAGED_INSTALL");
let disable_update_env_var = format!("{env_app_name}_DISABLE_UPDATE");

Copy link
Contributor

Choose a reason for hiding this comment

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

We should maybe also expose FLAT_INSTALL_LAYOUT as a raw option too? less certain of that

Copy link
Member

Choose a reason for hiding this comment

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

i would agree with this, in all my conversations with folks it's not clear that flat or not flat is preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about we circle back around to publicizing this around the time we also give developers full control over the install layout?

cargo-dist/templates/installer/installer.sh.j2 Outdated Show resolved Hide resolved
say "this installation will not be able to self-update"
local _retval=0
else
err "unable to create receipt directory at $RECEIPT_HOME"
Copy link
Contributor

Choose a reason for hiding this comment

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

"install receipt"

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'm happy to change it, but FWIW this is the text we already used before this PR. This isn't new, it just moved within the diff.

The new "unmanaged" installation mode allows for something close
to "untar and move on" installations. It disables modifying the
PATH and creation of environment files, and ensures all files are
installed into a single flat directory no matter what other
configuration is in place. It also disables installing a standalone
updater and the creation of an install receipt.

The new "don't install an updater/receipt" behaviour is also
controllable behind a new public environment variable,
$APPNAME_DISABLE_UPDATE.

Fixes #1400.
Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

think we're basically there, a few more nits

Comment on lines +66 to +74
if ($unmanaged_install) {
$NoModifyPath = $true
$install_updater = $false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "mode" variable should get applied first, and then the other overrides should kick in..?

Comment on lines +35 to +38
if [ -n "${UNMANAGED_INSTALL}" ]; then
NO_MODIFY_PATH=1
INSTALL_UPDATER=0
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto on mode should go first..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I think "unmanaged mode" is opinionated enough that it probably should override the individual options instead of the other way around. I see the individual options as being useful in normal mode, not for this.

@mistydemeo
Copy link
Contributor Author

I just realized that the shell installer does provide a CLI param for --no-modify-path, so I've removed the change where I removed that feature from powershell. Instead, I've just added the env override while retaining CLI support.

@mistydemeo mistydemeo merged commit 9e97a0c into main Sep 17, 2024
16 checks passed
@mistydemeo mistydemeo deleted the ci-mode branch September 17, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create "ci" mode for ephemeral environments, with bare minimum env assumptions
3 participants