habitat-sh / core Goto Github PK

The currently return type is Result<Vec<u8>>. This implies that the encrypted values are raw bytes, when in-fact they are bytes that represent a base64 encoded utf8 string. Either the return type should be changed to Result<String> (and comments added to indicate that String is base64 encoded) or the return value should be raw bytes, not base64-encoded.

This caused quite a bit of confusion.

Do the same configuration for this repo that we did for habitat.

There are several AtomicBools that are declared static mut and accessing static mut data requires unsafe code. However, atomics use interior mutability, so it's not necessary to declare these variable static mut; static will suffice and therefore the unsafe code to get/set these atomics can be removed.

Additionally, one of the atomics is named NO_COLOR with the comment:

// I am sorry this isn't named the other way; I can't get an atomic initializer that defaults to
// true. Them's the breaks.

This is no longer true; the following is legal since the introduction of const fn:

static COLOR: AtomicBool = AtomicBool::new(true);

The handle_signal function is registered to receive signals, but all it does currently is modify two static atomic variables to indicate that a signal is waiting to be processed and which signal it is. Consumer code calls check_for_signal, which resets the state of these variables before returning the type of signal that is waiting (or None).

In the event that multiple signals are received before check_for_signal is called, all but the last would be functionally ignored. Additionally, there is no way for the code calling check_for_signal to indicate whether it's successfully processed the signal.

Currently set_permissions and set_owner depend upon chmod and chown. However, on Windows, these functions are implemented as essentially no-ops. This leads to code in habitat and builder which is potentially very misleading.

Once the dependent code is fixed (see habitat-sh/habitat#5591 and habitat-sh/builder#627), the set_permissions and set_owner implementations should be removed from the Windows platform until we can provide proper implementations. This will require additional, nontrivial FFI work since Window's permissions and ownership models are different.

Same as habitat-sh/habitat#5806

#5580 accomplished this on Linux but the chmod and chown implementations for Windows in core are no op so that change has no effect on Windows. This will not be quite as simple as providing Windows implementations to those functions since the Windows APIs for accomplishing this are much different. I'm not sure exactly how this will look yet but I am filing this in this repo since this is where the behavior is needed and it will likely include changes here as well as core.