Skip to content

tiltfile: obsolesce multipleContainersPerPod flag, validate live updates better#3471

Merged
maiamcc merged 2 commits into
masterfrom
maiamcc/obsolesce-multiple-containers-flag
Jun 19, 2020
Merged

tiltfile: obsolesce multipleContainersPerPod flag, validate live updates better#3471
maiamcc merged 2 commits into
masterfrom
maiamcc/obsolesce-multiple-containers-flag

Conversation

@maiamcc

@maiamcc maiamcc commented Jun 16, 2020

Copy link
Copy Markdown
Contributor

Hello @jazzdan, @landism,

Please review the following commits I made in branch maiamcc/obsolesce-multiple-containers-flag:

1d6489f (2020-06-16 17:56:01 -0400)
tiltfile: obsolesce multipleContainersPerPod flag, validate live updates better

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@maiamcc
maiamcc requested review from jazzdan and landism June 16, 2020 21:56
Comment thread internal/tiltfile/tiltfile_state.go Outdated
// We won't collect container info on any subsequent containers (i.e. subsequent image
// targets), so will never be able to LiveUpdate them.
func (s *tiltfileState) checkForImpossibleLiveUpdates(m model.Manifest) error {
func (s *tiltfileState) validateLiveUpdates(manifests []model.Manifest) error {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this name seems pretty confusing next to validateLiveUpdate, which operates on a target

validateManifestsLiveUpdates? validateLiveUpdatesForManifests?

oy

@maiamcc
maiamcc merged commit 8f28bea into master Jun 19, 2020
@maiamcc
maiamcc deleted the maiamcc/obsolesce-multiple-containers-flag branch June 19, 2020 20:04
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.

2 participants