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

refactor!: de-zarfify maru-runner #73

Merged
merged 28 commits into from
May 22, 2024
Merged

refactor!: de-zarfify maru-runner #73

merged 28 commits into from
May 22, 2024

Conversation

Racer159
Copy link
Contributor

@Racer159 Racer159 commented May 8, 2024

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@Racer159
Copy link
Contributor Author

Racer159 commented May 8, 2024

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

@Racer159 Racer159 self-assigned this May 9, 2024
@Racer159 Racer159 added the breaking-change 💔 This introduces a breaking change label May 10, 2024
@Racer159 Racer159 marked this pull request as ready for review May 15, 2024 15:34
@Racer159 Racer159 requested a review from a team as a code owner May 15, 2024 15:34
@Racer159 Racer159 changed the title chore: de-zarfify maru-runner refactor!: de-zarfify maru-runner May 15, 2024
@UncleGedd
Copy link
Collaborator

UncleGedd commented May 15, 2024

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 defenseunicorns/pkg#80

@UncleGedd
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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)

Makefile Show resolved Hide resolved
src/cmd/root.go Outdated Show resolved Hide resolved
src/pkg/runner/actions.go Outdated Show resolved Hide resolved
@Racer159
Copy link
Contributor Author

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.

@Racer159 Racer159 requested a review from ericwyles May 20, 2024 14:46
@Racer159 Racer159 merged commit 635060c into main May 22, 2024
8 checks passed
@Racer159 Racer159 deleted the de-zarfify-maru-runner branch May 22, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change 💔 This introduces a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

De-dup types Remove dependency on Zarf
3 participants