search

defenseunicorns / maru-runner

The Unicorn Task Runner
Apache License 2.0
11 stars 1 forks source link

refactor!: de-zarfify maru-runner #73

Closed Racer159 closed 6 months ago

Racer159 commented 6 months ago

Description

This PR removes Zarf as a dependency of Maru, and proposes the following breaking changes to the library API surface:

  1. Creating a simple registration system for ./ prefixed apps
  2. Not reaching into os.Env within library code (i.e. when processing RUN_<VAR>)

This also proposes the following feature breaking changes:

  1. Drop support for files.

Related Issue

Fixes #23 Fixes #60

Type of change

Checklist before merging

Racer159 commented 6 months ago

still wip, there is some breakage with files and this still depends on message

UncleGedd commented 6 months ago

~Re: new deps. Looking at defenseunicorns/pkg, I see helpers but not exec or variables . Where are these deps coming from?~

~^ referring to the following entries from the go.mod:~

    github.com/defenseunicorns/pkg/exec v0.0.0-20240514195347-a77ce380de8c
    github.com/defenseunicorns/pkg/helpers v1.1.3-0.20240514195347-a77ce380de8c
    github.com/defenseunicorns/pkg/variables v0.0.0-20240514195347-a77ce380de8c

EDIT: nvm I found them https://github.com/defenseunicorns/pkg/pull/80

UncleGedd commented 6 months ago

I understand the registration system and removal of config.CmdPrefix, but I don't get the second breaking change: "Not reaching into os.Env within library code". Can we get more context there?

Racer159 commented 6 months ago

I understand the registration system and removal of config.CmdPrefix, but I don't get the second breaking change: "Not reaching into os.Env within library code". Can we get more context there?

This is a breakage for any users of Maru as a library - (which @ericwyles pointed out I was misinformed about uds-cli's integration of Maru) - basically we only pull the os.Env in the cmd package now so if you use Run directly you'd have to inject those env vars as set variables yourself.

We should discuss but I also think that uds-cli should switch from vendoring Maru to calling Run so that it can have more control and we don't need to have global config vars for all of the configuration.

UncleGedd commented 6 months ago

We should discuss but I also think that uds-cli should switch from vendoring Maru to calling Run so that it can have more control and we don't need to have global config vars for all of the configuration.

👍 I'm good with that

Racer159 commented 6 months ago

We should discuss but I also think that uds-cli should switch from vendoring Maru to calling Run so that it can have more control and we don't need to have global config vars for all of the configuration.

👍 I'm good with that

After discussing with @ericwyles yesterday I have changed my opinion back - we could do things to make it easier to run from library code (i.e. centralizing command flags) but they likely aren't necessary near term for what UDS CLI actually needs (likely a config var thing would work and we still should be able to control it well enough that vendoring apps like zarf / uds-cli don't step on each other in the globals)

Racer159 commented 6 months ago

Thinking of leaving prompt in for now (as dead code we can either cleanup or implement later) - I implemented generics though for the Zarf specific variable values that are carried through.