lix-project/lix-install-action
1
0
Fork 0
Code Issues 11 Pull requests Packages Projects Releases Wiki Activity

Typescript rewrite #34

Merged
Hoverbear merged 25 commits from hoverbear/ds-893-make-installer-action-use-typescript into main 2023-07-17 18:13:56 +00:00
Conversation 4 Commits 25 Files changed 16 +18010 -242
Hoverbear commented 2023-07-11 17:34:49 +00:00 (Migrated from github.com)
Copy link
Description

Port the action to typescript!

This will let us avoid having bash in this part of the codebase. It also gives us the space to add some more intelligence to the action eg richer logging: https://www.npmjs.com/package/@actions/core

This is a fairly basic and direct port, more a starting point than any added features.

Checklist
  • Tested changes against a test repository
  • Added or updated relevant documentation (leave unchecked if not applicable)
  • (If this PR is for a release) Updated README to point to the new tag (leave unchecked if not applicable)
##### Description Port the action to typescript! This will let us avoid having bash in this part of the codebase. It also gives us the space to add some more intelligence to the action eg richer logging: https://www.npmjs.com/package/@actions/core This is a fairly basic and direct port, more a starting point than any added features. ##### Checklist - [ ] Tested changes against a test repository - [ ] Added or updated relevant documentation (leave unchecked if not applicable) - [ ] (If this PR is for a release) Updated README to point to the new tag (leave unchecked if not applicable)
cole-h (Migrated from github.com) reviewed 2023-07-13 17:18:49 +00:00
action.yml
cole-h (Migrated from github.com) commented 2023-07-13 17:18:48 +00:00
Copy link

(while we're here)

    description: A local `nix-installer` binary root, overrides any settings which change the `nix-installer` used (binaries should be named `nix-installer-$ARCH-$OS`, eg. `nix-installer-x86_64-linux`)

or

    description: A local `nix-installer` binary root, overrides any settings which change the `nix-installer` used (binaries should be named `nix-installer-$SYSTEM`, eg. `nix-installer-x86_64-linux`)
(while we're here) ```suggestion description: A local `nix-installer` binary root, overrides any settings which change the `nix-installer` used (binaries should be named `nix-installer-$ARCH-$OS`, eg. `nix-installer-x86_64-linux`) ``` or ```suggestion description: A local `nix-installer` binary root, overrides any settings which change the `nix-installer` used (binaries should be named `nix-installer-$SYSTEM`, eg. `nix-installer-x86_64-linux`) ```
Hoverbear commented 2023-07-13 17:23:30 +00:00 (Migrated from github.com)
Copy link

(I invited some testers via social media, not going to merge this for a day at least)

(I invited some testers via social media, not going to merge this for a day at least)
cole-h (Migrated from github.com) reviewed 2023-07-13 17:33:29 +00:00
src/main.ts
@ -0,0 +16,4 @@
extra_args: string | null;
extra_conf: string[] | null;
github_token: string | null;
// TODO: linux_init
cole-h (Migrated from github.com) commented 2023-07-13 17:33:28 +00:00
Copy link

Do we want to do this now or later?

Do we want to do this now or later?
cole-h (Migrated from github.com) reviewed 2023-07-13 17:35:09 +00:00
src/main.ts
@ -0,0 +136,4 @@
this.diagnostic_endpoint;
}
// TODO: Error if the user uses these on not-MacOS
cole-h (Migrated from github.com) commented 2023-07-13 17:35:09 +00:00
Copy link

Same here, should we do this now or later?

Same here, should we do this now or later?
cole-h (Migrated from github.com) reviewed 2023-07-13 17:35:40 +00:00
src/main.ts
@ -0,0 +177,4 @@
execution_env.NIX_INSTALLER_LOG_DIRECTIVES = this.log_directives;
}
// TODO: Error if the user uses these on MacOS
cole-h (Migrated from github.com) commented 2023-07-13 17:35:40 +00:00
Copy link

I think you probably meant "on not-MacOS", since launchd is their only choice. (Also, same as above -- now or later?)

I think you probably meant "on not-MacOS", since launchd is their only choice. (Also, same as above -- now or later?)
cole-h (Migrated from github.com) reviewed 2023-07-13 17:36:18 +00:00
src/main.ts
cole-h (Migrated from github.com) commented 2023-07-13 17:36:18 +00:00
Copy link

Improve what part? The necessity of trusted-users, or the $USER detection?

Improve what part? The necessity of trusted-users, or the `$USER` detection?
cole-h (Migrated from github.com) reviewed 2023-07-13 17:37:29 +00:00
src/main.ts
@ -0,0 +211,4 @@
execution_env.NIX_INSTALLER_EXTRA_CONF = extra_conf;
if (process.env.ACT && !process.env.NOT_ACT) {
actions_core.info(
cole-h (Migrated from github.com) commented 2023-07-13 17:37:28 +00:00
Copy link

It doesn't look like we actually change anything in extra-conf at this point, so I think that point can be removed (unless it was just an accidental omission of the extra-conf modification).

It doesn't look like we actually change anything in extra-conf at this point, so I think that point can be removed (unless it was just an accidental omission of the extra-conf modification).
cole-h (Migrated from github.com) reviewed 2023-07-13 17:39:35 +00:00
src/main.ts
@ -0,0 +238,4 @@
args.concat(extra_args);
}
const merged_env = {
cole-h (Migrated from github.com) commented 2023-07-13 17:39:34 +00:00
Copy link

I wonder how this behaves in the case where a user specifies env: NIX_INSTALLER_EXTRA_CONF: "asdf" and with: extra-conf: "foo"... Will it choose the input over the env, or error because there are two envs with the same name but conflicting values?

I wonder how this behaves in the case where a user specifies `env: NIX_INSTALLER_EXTRA_CONF: "asdf"` and `with: extra-conf: "foo"`... Will it choose the input over the env, or error because there are two envs with the same name but conflicting values?
Hoverbear (Migrated from github.com) reviewed 2023-07-13 17:40:31 +00:00
src/main.ts
@ -0,0 +238,4 @@
args.concat(extra_args);
}
const merged_env = {
Hoverbear (Migrated from github.com) commented 2023-07-13 17:40:31 +00:00
Copy link

In theory a user could not modify the env provided to the javascript!

In theory a user could not modify the env provided to the javascript!
cole-h (Migrated from github.com) reviewed 2023-07-13 17:43:37 +00:00
src/main.ts
cole-h (Migrated from github.com) commented 2023-07-13 17:43:37 +00:00
Copy link

Looks to me like the first part of this was done (I see a this.set_github_path(); above :D)

Looks to me like the first part of this was done (I see a `this.set_github_path();` above :D)
cole-h (Migrated from github.com) reviewed 2023-07-13 17:44:53 +00:00
src/main.ts
cole-h (Migrated from github.com) commented 2023-07-13 17:44:53 +00:00
Copy link

How so? I think the smartest it could be is "only check for the receipt when needed", but maybe I'm forgetting something.

How so? I think the smartest it could be is "only check for the receipt when needed", but maybe I'm forgetting something.
Hoverbear (Migrated from github.com) reviewed 2023-07-13 17:47:29 +00:00
src/main.ts
Hoverbear (Migrated from github.com) commented 2023-07-13 17:47:29 +00:00
Copy link

I was kinda hoping there was some way to check the installed version is the desired version but at this time it seems overly complex. I'll remove the TODO

I was kinda hoping there was some way to check the installed version is the desired version but at this time it seems overly complex. I'll remove the TODO
Hoverbear (Migrated from github.com) reviewed 2023-07-13 17:48:55 +00:00
src/main.ts
@ -0,0 +16,4 @@
extra_args: string | null;
extra_conf: string[] | null;
github_token: string | null;
// TODO: linux_init
Hoverbear (Migrated from github.com) commented 2023-07-13 17:48:55 +00:00
Copy link

Might as well

Might as well
Hoverbear (Migrated from github.com) reviewed 2023-07-13 17:51:16 +00:00
src/main.ts
@ -0,0 +16,4 @@
extra_args: string | null;
extra_conf: string[] | null;
github_token: string | null;
// TODO: linux_init
Hoverbear (Migrated from github.com) commented 2023-07-13 17:51:16 +00:00
Copy link

Thinking on this more, I actually don't know if we should do this. While MacOS might not have an init setting (since we need the services for volume mounting), a future OS we add, such as FreeBSD, will likely have this option.

Thinking on this more, I actually don't know if we should do this. While MacOS might not have an init setting (since we need the services for volume mounting), a future OS we add, such as FreeBSD, will likely have this option.
Hoverbear (Migrated from github.com) reviewed 2023-07-13 17:58:51 +00:00
src/main.ts
Hoverbear (Migrated from github.com) commented 2023-07-13 17:58:51 +00:00
Copy link

I think this is fine, actually!

I think this is fine, actually!
Hoverbear (Migrated from github.com) reviewed 2023-07-13 18:01:31 +00:00
src/main.ts
Hoverbear (Migrated from github.com) commented 2023-07-13 18:01:31 +00:00
Copy link

Ya I need to make the other side of this, let me cook a PR.

Ya I need to make the other side of this, let me cook a PR.
Hoverbear (Migrated from github.com) reviewed 2023-07-13 18:07:04 +00:00
src/main.ts
Hoverbear (Migrated from github.com) commented 2023-07-13 18:07:04 +00:00
Copy link

https://github.com/DeterminateSystems/nix-installer/pull/568

https://github.com/DeterminateSystems/nix-installer/pull/568
cole-h (Migrated from github.com) reviewed 2023-07-13 18:17:11 +00:00
src/main.ts
@ -0,0 +471,4 @@
let resolved_nix_installer_url = null;
let num_set = 0;
if (nix_installer_branch !== null) {
cole-h (Migrated from github.com) commented 2023-07-13 18:17:11 +00:00
Copy link

Very minor nit: I wonder if it might be a little nicer to set the URL we'll be using in the checks above, and then return that_url after we've checked that only 1 of those options is set.

Like, let url = "default install url"; if branch != null, url = "branch install url"; or something.

Very minor nit: I wonder if it might be a little nicer to set the URL we'll be using in the checks above, and then `return that_url` after we've checked that only 1 of those options is set. Like, `let url = "default install url"; if branch != null, url = "branch install url";` or something.
cole-h (Migrated from github.com) reviewed 2023-07-13 18:19:10 +00:00
src/main.ts
@ -0,0 +238,4 @@
args.concat(extra_args);
}
const merged_env = {
cole-h (Migrated from github.com) commented 2023-07-13 18:19:09 +00:00
Copy link

I meant providing both an env: and a with: to the action that touch the same options. But I don't think it's worth being super defensive about until we see issues about it.

I meant providing both an `env:` and a `with:` to the action that touch the same options. But I don't think it's worth being super defensive about until we see issues about it.
Hoverbear (Migrated from github.com) reviewed 2023-07-14 15:29:08 +00:00
src/main.ts
@ -0,0 +238,4 @@
args.concat(extra_args);
}
const merged_env = {
Hoverbear (Migrated from github.com) commented 2023-07-14 15:29:08 +00:00
Copy link

Ahhh ya, fair.

Ahhh ya, fair.
Hoverbear commented 2023-07-14 16:16:33 +00:00 (Migrated from github.com)
Copy link

Added some handling of if $GITHUB_PATH has already been written (by the installer running as root) so this won't need tight version cadence.

Added some handling of if `$GITHUB_PATH` has already been written (by the installer running as root) so this won't need tight version cadence.
👍 1
Hoverbear commented 2023-07-14 16:57:32 +00:00 (Migrated from github.com)
Copy link

There is a bit of a complication here:

  • We release a new version of this action which does the GITHUB_PATH update
  • We release a version of nix-installer in the future which does not do the update
  • Folks running the old version of the nix-installer-action have their CI break because their action isn't doing the GITHUB_PATH update.

We'll need a transitionary period where we have GITHUB_PATH being set in both places, and monitor for use of our old versions for a bit. After a few versions (and no public repos using the old action version) we can remove the old GITHUB_PATH code.

I think in the meantime we should have the nix-installer output a warning to update the nix-installer-action if it detects it's in CI.

There is a bit of a complication here: * We release a new version of this action which does the `GITHUB_PATH` update * We release a version of `nix-installer` in the future which does not do the update * Folks running the old version of the `nix-installer-action` have their CI break because their action isn't doing the `GITHUB_PATH` update. We'll need a transitionary period where we have `GITHUB_PATH` being set in both places, and monitor for use of our old versions for a bit. After a few versions (and no public repos using the old action version) we can remove the old `GITHUB_PATH` code. I think in the meantime we should have the `nix-installer` output a warning to update the `nix-installer-action` if it detects it's in CI.
cole-h (Migrated from github.com) approved these changes 2023-07-14 17:56:26 +00:00
cole-h (Migrated from github.com) left a comment
Copy link

Looking good!

Looking good!
Hoverbear commented 2023-07-14 19:31:45 +00:00 (Migrated from github.com)
Copy link

I think it should gracefully handle https://github.com/DeterminateSystems/nix-installer-action/pull/34#issuecomment-1636127448 now.

I think it should gracefully handle https://github.com/DeterminateSystems/nix-installer-action/pull/34#issuecomment-1636127448 now.
Sign in to join this conversation.
Reviewers
No reviewers
cole-h
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: lix-project/lix-install-action#34
No description provided.
Delete branch "hoverbear/ds-893-make-installer-action-use-typescript"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?