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

engine: make Tiltfile.Spec.Args single source of truth #5226

Merged
merged 3 commits into from
Nov 25, 2021

Conversation

landism
Copy link
Member

@landism landism commented Nov 24, 2021

Old Behavior

When we run tilt args, it goes through a server.go api which edits the Tiltfile API object and also dispatches a SetTiltfileArgsAction to the engine to update EngineState.UserConfigState.Args.

New Behavior

The engine sets EngineState.UserConfigState.Args based on the main Tiltfile's Spec.Args, with no separate action needed.

This means we can just edit the Tiltfile API object without having to have a Store.

@landism landism requested a review from nicks November 24, 2021 19:45
@landism landism changed the title engine: remove engine from tilt args flow engine: make Tiltfile.Spec.Args single source of truth Nov 24, 2021
@landism
Copy link
Member Author

landism commented Nov 24, 2021

Per Nick's comment, I've reverted the EngineState changes.

The main effect of this PR is now that UpsertTiltfileAction also updates EngineState.UserConfigState.Args, which means we can simply edit the Tiltfile args in the API and don't also have to dispatch a separate SetTiltfileArgsAction (which means we can change args w/o having an instance of Store).

@@ -18,6 +18,10 @@ func HandleTiltfileUpsertAction(state *store.EngineState, action TiltfileUpsertA
}
}

if mn == model.MainTiltfileManifestName {
state.UserConfigState.Args = action.Tiltfile.Spec.Args
Copy link
Member

Choose a reason for hiding this comment

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

haha i like this solution

@landism landism merged commit 7ec0bc0 into master Nov 25, 2021
@landism landism deleted the matt/remove_UserConfigState branch November 25, 2021 01:35
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.

None yet

2 participants