Skip to content

feat: add --manifest-source flag to run and deploy commands - #630

Merged
srtaalej merged 16 commits into
mainfrom
ale-manifest-source-flag
Sep 11, 2026
Merged

feat: add --manifest-source flag to run and deploy commands#630
srtaalej merged 16 commits into
mainfrom
ale-manifest-source-flag

Conversation

@srtaalej

@srtaalej srtaalej commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a --manifest-source=project|remote flag to slack run and slack deploy commands
  • When manifest sync detects differences during run/deploy, this flag allows non-interactive resolution without requiring slack manifest sync to be run separately
  • Skips the "Overwrite manifest on app settings?" confirmation prompt during install when --manifest-source is set — project auto-approves the overwrite, remote skips it entirely
  • Updates the non-TTY error remediation to reference --manifest-source instead of --force/--force-remote (which are only available on manifest sync)

Closes #628

Test plan

  • make lint passes
  • make test passes
  • Manual test: slack run --manifest-source=project pushes local manifest without prompting
  • Manual test: slack run --manifest-source=remote pulls app settings without prompting
  • Manual test: slack deploy --manifest-source=project works in non-TTY (CI) environments
  • Manual test: slack run --manifest-source=invalid returns a clear validation error
  • Manual test: slack run --manifest-source=remote skips the "Overwrite manifest?" prompt on reinstall

@srtaalej
srtaalej requested a review from a team as a code owner August 10, 2026 17:49
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.42857% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.20%. Comparing base (eb6139d) to head (5e21ac5).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
internal/pkg/apps/install.go 60.00% 2 Missing ⚠️
internal/config/manifest.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #630      +/-   ##
==========================================
- Coverage   78.21%   78.20%   -0.01%     
==========================================
  Files         239      239              
  Lines       18149    18176      +27     
==========================================
+ Hits        14195    14215      +20     
- Misses       3954     3961       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@srtaalej srtaalej self-assigned this Aug 10, 2026
@srtaalej srtaalej added enhancement M-T: A feature request for new functionality semver:minor Use on pull requests to describe the release version increment labels Aug 10, 2026
@srtaalej
srtaalej requested a review from zimeg August 17, 2026 20:57
@srtaalej srtaalej added this to the Next Release milestone Aug 17, 2026
@zimeg zimeg modified the milestones: v4.7.0, Next Release Aug 28, 2026

@zimeg zimeg left a comment

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.

@srtaalej I appreciate the most patient request for review 🙏 ✨ I'm requesting a few changes with hopes these comments move this in evermore stable directions. I call out:

  • Removing --force-remote flag altogether alongside this change: I'd like to avoid multiple options unless follow up is planned to remove this too?
  • Favoring the existing terms and implementations of manifest source: Our configuration file has some logic we might reuse here!

If I can share more to these please let me know! I'm optimistic we include this in upcoming release 🚀 🔮

Comment thread internal/manifest/sync.go Outdated
Comment on lines +95 to +96
style.CommandText("--manifest-source=project / --force"),
style.CommandText("--manifest-source=remote / --force-remote"),

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.

🪓 question: Are we alright to replace the --force and --force-remote options altogether while the sync command is under experiment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes i think thats a nice consolidation!

Comment thread internal/config/config.go Outdated
Comment on lines 57 to 58
ManifestSourceFlag string
LogstashHostResolved string

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.

🧮 suggestion: Let's keep this in alphabetical order!

Comment thread internal/cmdutil/flags.go Outdated
Comment on lines +41 to +44
const (
ManifestSourceProject = "project"
ManifestSourceRemote = "remote"
)

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.

Suggested change
const (
ManifestSourceProject = "project"
ManifestSourceRemote = "remote"
)
const (
ManifestSourceProject = "local"
ManifestSourceRemote = "remote"
)

🪬 suggestion(blocking): Earlier suggestion might've hinted at "project" terms but we should match existing configuration options I realize. Perhaps reusing logic from this package instead of validations here?

const (
ManifestSourceLocal ManifestSource = "local"
ManifestSourceRemote ManifestSource = "remote"
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree!

Comment thread cmd/platform/deploy.go
Comment on lines +62 to +64
if err := cmdutil.ValidateManifestSourceFlag(clients); err != nil {
return err
}

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.

Suggested change
if err := cmdutil.ValidateManifestSourceFlag(clients); err != nil {
return err
}

🪓 quibble: I'd favor this validation happening with the switch case in internal/manifest/sync.go to avoid duplicate checks in code, although I understand this might error earlier.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we actually do need the check to happen in run and deploy as well otherwise invalid values in the flag will fall through

@srtaalej
srtaalej requested review from zimeg and removed request for zimeg September 8, 2026 21:13
@srtaalej
srtaalej requested a review from zimeg September 8, 2026 21:51

@zimeg zimeg left a comment

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.

💬 @srtaalej Handfulls more comment around consolidating on config and flag options across commands! The PRs looking solid but I'd like to keep internal representations together with this!

Comment thread internal/config/config.go Outdated
@@ -55,6 +55,7 @@ type Config struct {
ForceFlag bool
ForceRemoteFlag bool

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.

Suggested change
ForceRemoteFlag bool

🪓 note: I'm eager to replace this as part of these changes while the initial implementation is under experiment still. IMHO a replacement of these instances makes safer review without handling multiple cases.

Comment thread internal/manifest/sync.go Outdated
Comment thread internal/manifest/sync.go
if err != nil {
return nil, err
}
case !clients.IO.IsTTY():

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.

Suggested change
case config.ManifestSource(clients.Config.ManifestSourceFlag).Exists() && !config.ManifestSource(clients.Config.ManifestSourceFlag).IsValid():
return nil, slackerror.New(slackerror.ErrInvalidFlag).
WithMessage("Invalid value %q for %s flag", clients.Config.ManifestSourceFlag, style.CommandText("--manifest-source")).
WithRemediation("Valid values are %s or %s",
style.Highlight(string(config.ManifestSourceLocal)),
style.Highlight(string(config.ManifestSourceRemote)),
)
case !clients.IO.IsTTY():

🍾 suggestion: We might introduce a new "IsValid" method to the manifest source configurations to compare and error here?

Comment thread internal/manifest/sync.go Outdated
Comment on lines +39 to +47
if v := clients.Config.ManifestSourceFlag; v != "" && !(v == string(config.ManifestSourceLocal) || v == string(config.ManifestSourceRemote)) {
return nil, slackerror.New(slackerror.ErrInvalidFlag).
WithMessage("Invalid value %q for %s flag", v, style.CommandText("--manifest-source")).
WithRemediation("Valid values are %s or %s",
style.Highlight(string(config.ManifestSourceLocal)),
style.Highlight(string(config.ManifestSourceRemote)),
)
}

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.

Suggested change
if v := clients.Config.ManifestSourceFlag; v != "" && !(v == string(config.ManifestSourceLocal) || v == string(config.ManifestSourceRemote)) {
return nil, slackerror.New(slackerror.ErrInvalidFlag).
WithMessage("Invalid value %q for %s flag", v, style.CommandText("--manifest-source")).
WithRemediation("Valid values are %s or %s",
style.Highlight(string(config.ManifestSourceLocal)),
style.Highlight(string(config.ManifestSourceRemote)),
)
}

🪓 suggestion: Am hoping the checks below can cover this validation without causing issue?

🌲 note: I lean towards keeping validation in cmd overall which I might also have suggestion these files shouldn't be in internal... But that can be a later change!

Comment thread internal/cmdutil/flags.go
Comment on lines +41 to +56
// ValidateManifestSourceFlag checks that --manifest-source has a valid value if set
func ValidateManifestSourceFlag(clients *shared.ClientFactory) error {
v := clients.Config.ManifestSourceFlag
if v == "" {
return nil
}
if !(v == string(config.ManifestSourceLocal) || v == string(config.ManifestSourceRemote)) {
return slackerror.New(slackerror.ErrInvalidFlag).
WithMessage("Invalid value %q for %s flag", v, style.CommandText("--manifest-source")).
WithRemediation("Valid values are %s or %s",
style.Highlight(string(config.ManifestSourceLocal)),
style.Highlight(string(config.ManifestSourceRemote)),
)
}
return nil
}

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.

👁️‍🗨️ note: Adjacent comment suggests adding "IsValid" to the manifest source configurations that I think might be useful instead of flag specific checks here?

👾 note: I'm not so confident with PreRunE flag checks but am thinking that might return both truth values with an optional error:

func (ms ManifestSource) IsValid() (bool, error)

srtaalej and others added 2 commits September 10, 2026 16:13

@zimeg zimeg left a comment

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.

@srtaalej Kind thanks for all the iterations once more and LGTM let's get this merged 🚢 💨

Comment thread internal/config/config.go
Comment on lines -56 to +57
ForceRemoteFlag bool
LogstashHostResolved string
ManifestSourceFlag string

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.

🌟 praise: Super appreciate keeping shared flags here!

Comment thread internal/pkg/apps/install.go Outdated
Comment on lines +704 to +709
if clients.Config.ManifestSourceFlag == string(config.ManifestSourceRemote) {
return false, nil
}
if clients.Config.ManifestSourceFlag == string(config.ManifestSourceLocal) {
return true, nil
}

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.

🏁 quibble(non-blocking): Our flags overall might need some care but it'd be cool to use something as:

config.ManifestSource(clients.Config.ManifestSourceFlag).Equals(config.ManifestSourceRemote)

@srtaalej
srtaalej merged commit de89617 into main Sep 11, 2026
14 checks passed
@srtaalej
srtaalej deleted the ale-manifest-source-flag branch September 11, 2026 19:40
@zimeg zimeg added the changelog Use on updates to be included in the release notes label Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog Use on updates to be included in the release notes enhancement M-T: A feature request for new functionality semver:minor Use on pull requests to describe the release version increment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add --force/--force-remote flags to run and deploy commands

2 participants