Skip to content

Commit

Permalink
engine: don't pass user args to extensions (#4897)
Browse files Browse the repository at this point in the history
fixes #4892
  • Loading branch information
nicks committed Aug 27, 2021
1 parent 1aa4fa2 commit cb937cd
Show file tree
Hide file tree
Showing 6 changed files with 286 additions and 213 deletions.
215 changes: 215 additions & 0 deletions internal/controllers/core/tiltfile/reducers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
package tiltfile

import (
"context"

"github.com/tilt-dev/tilt/internal/sliceutils"
"github.com/tilt-dev/tilt/internal/store"
"github.com/tilt-dev/tilt/pkg/logger"
"github.com/tilt-dev/tilt/pkg/model"
)

func HandleConfigsReloadStarted(
ctx context.Context,
state *store.EngineState,
event ConfigsReloadStartedAction,
) {
ms, ok := state.TiltfileStates[event.Name]
if !ok {
return
}

status := model.BuildRecord{
StartTime: event.StartTime,
Reason: event.Reason,
Edits: event.FilesChanged,
SpanID: event.SpanID,
}
ms.CurrentBuild = status
state.RemoveFromTriggerQueue(event.Name)
state.StartedTiltfileLoadCount++
}

// In the original Tilt architecture, the Tiltfile contained
// the whole engine state. Reloading the tiltfile re-created that
// state from scratch.
//
// We've moved towards a more modular architecture, but many of the tilt data
// models aren't modular. For example, if two Tiltfiles set UpdateSettings,
// it's not clear which one "wins" or how their preferences combine.
//
// In the long-term, Tiltfile settings should only take affect in objects created
// by that Tiltfile. (e.g., WatchSettings only affects FileWatches created by
// that Tiltfile.)
//
// In the medium-term, we resolve this in the EngineState in three different ways:
// 1) If a data structure supports merging (like the Manifest map), do a merge.
// 2) If merging fails (like if two Tiltfiles define the same Manifest), log an Error
// and try to do something resonable.
// 3) If a data structure does not support merging (like UpdateSettings), only
// accept that data structure from the "main" tiltfile.
func HandleConfigsReloaded(
ctx context.Context,
state *store.EngineState,
event ConfigsReloadedAction,
) {
isMainTiltfile := event.Name == model.MainTiltfileManifestName

manifests := event.Manifests
loadedManifestNames := map[model.ManifestName]bool{}
for i, m := range manifests {
loadedManifestNames[m.Name] = true

// Properly annotate the manifest with the source tiltfile.
m.SourceTiltfile = event.Name
manifests[i] = m
}

ms, ok := state.TiltfileStates[event.Name]
if !ok {
return
}
b := ms.CurrentBuild

// Remove pending file changes that were consumed by this build.
for _, status := range ms.BuildStatuses {
status.ClearPendingChangesBefore(b.StartTime)
}

// Track the new secrets and go back to scrub them.
newSecrets := model.SecretSet{}
for k, v := range event.Secrets {
_, exists := state.Secrets[k]
if !exists {
newSecrets[k] = v
}
}

// Add all secrets, even if we failed.
state.Secrets.AddAll(event.Secrets)

// Retroactively scrub secrets
state.LogStore.ScrubSecretsStartingAt(newSecrets, event.CheckpointAtExecStart)

// Add team id if it exists, even if execution failed.
if isMainTiltfile && (event.TeamID != "" || event.Err == nil) {
state.TeamID = event.TeamID
}

// Only set the metrics settings from the tiltfile if the mode
// hasn't been configured from elsewhere.
if isMainTiltfile && state.MetricsServing.Mode == model.MetricsDefault {
// Add metrics if it exists, even if execution failed.
if event.MetricsSettings.Enabled || event.Err == nil {
state.MetricsSettings = event.MetricsSettings
}
}

// if the ConfigsReloadedAction came from a unit test, there might not be a current build
if !b.Empty() {
b.FinishTime = event.FinishTime
b.Error = event.Err

if b.SpanID != "" {
b.WarningCount = len(state.LogStore.Warnings(b.SpanID))
}

ms.AddCompletedBuild(b)
}
ms.CurrentBuild = model.BuildRecord{}
if event.Err != nil {
// When the Tiltfile had an error, we want to differentiate between two cases:
//
// 1) You're running `tilt up` for the first time, and a local() command
// exited with status code 1. Partial results (like enabling features)
// would be helpful.
//
// 2) You're running 'tilt up' in the happy state. You edit the Tiltfile,
// and introduce a syntax error. You don't want partial results to wipe out
// your "good" state.

// Watch any new config files in the partial state.
state.TiltfileConfigPaths[event.Name] = sliceutils.AppendWithoutDupes(state.TiltfileConfigPaths[event.Name], event.ConfigFiles...)

if isMainTiltfile {
// Enable any new features in the partial state.
if len(state.Features) == 0 {
state.Features = event.Features
} else {
for feature, val := range event.Features {
if val {
state.Features[feature] = val
}
}
}
}
return
}

// Make sure all the new manifests are in the EngineState.
for _, m := range manifests {
mt, ok := state.ManifestTargets[m.ManifestName()]
if ok && mt.Manifest.SourceTiltfile != event.Name {
logger.Get(ctx).Errorf("Resource defined in two tiltfiles: %s, %s", event.Name, mt.Manifest.SourceTiltfile)
continue
}

if !ok {
mt = store.NewManifestTarget(m)
}

configFilesThatChanged := ms.LastBuild().Edits
old := mt.Manifest
mt.Manifest = m

if model.ChangesInvalidateBuild(old, m) {
// Manifest has changed such that the current build is invalid;
// ensure we do an image build so that we apply the changes
ms := mt.State
ms.BuildStatuses = make(map[model.TargetID]*store.BuildStatus)
ms.PendingManifestChange = event.FinishTime
ms.ConfigFilesThatCausedChange = configFilesThatChanged
}
state.UpsertManifestTarget(mt)
}

// Go through all the existing manifest targets. If they were from this
// Tiltfile, but were removed from the latest Tiltfile execution, delete them.
for _, mt := range state.Targets() {
m := mt.Manifest

if m.SourceTiltfile == event.Name {
if !loadedManifestNames[m.Name] {
delete(state.ManifestTargets, m.Name)
}
continue
}
}

// Create a new definition order that merges:
// the existing definition order, and
// any new manifests.
newOrder := append([]model.ManifestName{}, state.ManifestDefinitionOrder...)
newOrderSet := make(map[model.ManifestName]bool)
for _, name := range newOrder {
newOrderSet[name] = true
}
for _, newManifest := range manifests {
if !newOrderSet[newManifest.Name] {
newOrder = append(newOrder, newManifest.Name)
}
}

state.ManifestDefinitionOrder = newOrder
state.TiltfileConfigPaths[event.Name] = event.ConfigFiles

// Global state that's only configurable from the main manifest.
if isMainTiltfile {
state.Features = event.Features
state.TelemetrySettings = event.TelemetrySettings
state.VersionSettings = event.VersionSettings
state.AnalyticsTiltfileOpt = event.AnalyticsTiltfileOpt
state.UpdateSettings = event.UpdateSettings
state.DockerPruneSettings = event.DockerPruneSettings
}
}
10 changes: 8 additions & 2 deletions internal/engine/configs/configs_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,13 @@ func (cc *ConfigsController) needsBuild(ctx context.Context, st store.RStore) (*
reason = reason.With(model.BuildReasonFlagChangedFiles)
}

if state.UserConfigState.ArgsChangeTime.After(lastStartTime) {
// Only the main tiltfile may read tilt configs and args.
userConfigState := state.UserConfigState
if name != model.MainTiltfileManifestName {
userConfigState = model.UserConfigState{}
}

if userConfigState.ArgsChangeTime.After(lastStartTime) {
reason = reason.With(model.BuildReasonFlagTiltfileArgs)
}

Expand All @@ -99,7 +105,7 @@ func (cc *ConfigsController) needsBuild(ctx context.Context, st store.RStore) (*
Name: name,
FilesChanged: filesChanged,
BuildReason: reason,
UserConfigState: state.UserConfigState,
UserConfigState: userConfigState,
TiltfilePath: tf.Spec.Path,
CheckpointAtExecStart: state.LogStore.Checkpoint(),
}, true
Expand Down
50 changes: 50 additions & 0 deletions internal/engine/configs/configs_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/util/workqueue"
"sigs.k8s.io/controller-runtime/pkg/handler"
Expand Down Expand Up @@ -42,6 +43,7 @@ func TestConfigsController(t *testing.T) {
f.addManifest("fe")

bar := manifestbuilder.New(f, "bar").WithK8sYAML(testyaml.SanchoYAML).Build()
bar.SourceTiltfile = model.MainTiltfileManifestName
f.setManifestResult(bar)
f.onChange()
f.popQueue()
Expand Down Expand Up @@ -125,7 +127,46 @@ func TestErrorLog(t *testing.T) {
assert.Contains(f.T(), f.st.out.String(), "ERROR LEVEL: The goggles do nothing!")
}

func TestExtensionTiltfile(t *testing.T) {
f := newCCFixture(t)
defer f.TearDown()

// Initialize the Main tiltfile
f.st.WithState(func(es *store.EngineState) {
es.UserConfigState = model.UserConfigState{
Args: []string{"fe"},
}
})
f.addManifest("fe")

bar := manifestbuilder.New(f, "fe").WithK8sYAML(testyaml.SanchoYAML).Build()
f.setManifestResult(bar)
f.onChange()
f.popQueue()
f.popQueue()

// Add an extension tiltfile.
f.st.WithState(func(es *store.EngineState) {
tiltfiles.HandleTiltfileUpsertAction(es, tiltfiles.TiltfileUpsertAction{
Tiltfile: &v1alpha1.Tiltfile{
ObjectMeta: metav1.ObjectMeta{
Name: "ext",
},
Spec: v1alpha1.TiltfileSpec{
Path: "extpath",
},
},
})
})
f.onChange()

entry := f.bs.Entry()
assert.Equal(t, "ext", string(entry.Name))
assert.Equal(t, model.UserConfigState{}, entry.UserConfigState)
}

type testStore struct {
ctx context.Context
*store.TestingStore
out *bytes.Buffer
start *ctrltiltfile.ConfigsReloadStartedAction
Expand Down Expand Up @@ -153,11 +194,17 @@ func (s *testStore) Dispatch(action store.Action) {

start, ok := action.(ctrltiltfile.ConfigsReloadStartedAction)
if ok {
state := s.LockMutableStateForTesting()
ctrltiltfile.HandleConfigsReloadStarted(s.ctx, state, start)
s.UnlockMutableState()
s.start = &start
}

end, ok := action.(ctrltiltfile.ConfigsReloadedAction)
if ok {
state := s.LockMutableStateForTesting()
ctrltiltfile.HandleConfigsReloaded(s.ctx, state, end)
s.UnlockMutableState()
s.end = &end
}

Expand All @@ -179,6 +226,7 @@ type ccFixture struct {
docker *docker.FakeClient
tfr *ctrltiltfile.Reconciler
q workqueue.RateLimitingInterface
bs *ctrltiltfile.BuildSource
}

func newCCFixture(t *testing.T) *ccFixture {
Expand All @@ -194,6 +242,7 @@ func newCCFixture(t *testing.T) *ccFixture {
cc := NewConfigsController(tc, buildSource)
ctx, _, _ := testutils.CtxAndAnalyticsForTest()
ctx, cancel := context.WithCancel(ctx)
st.ctx = ctx
_ = buildSource.Start(ctx, handler.Funcs{}, q)

// configs_controller uses state.RelativeTiltfilePath, which is relative to wd
Expand All @@ -219,6 +268,7 @@ func newCCFixture(t *testing.T) *ccFixture {
docker: d,
tfr: tfr,
q: q,
bs: buildSource,
}
}

Expand Down

0 comments on commit cb937cd

Please sign in to comment.