-
Notifications
You must be signed in to change notification settings - Fork 46
feat: add --manifest-source flag to run and deploy commands #630
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
base: main
Are you sure you want to change the base?
Changes from all commits
b021d22
a8d89ea
7ab29be
4837889
6ec8dc7
d327435
392db1e
a965364
958b568
edf1599
0cf12c1
6fc14df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -55,6 +55,7 @@ type Config struct { | |||
| ForceFlag bool | ||||
| ForceRemoteFlag bool | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
🪓 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. |
||||
| LogstashHostResolved string | ||||
| ManifestSourceFlag string | ||||
| NoColor bool | ||||
| RuntimeFlag string | ||||
| RuntimeName string | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,6 +36,15 @@ type SyncResult struct { | |||||||||||||||||
| // both manifests, computes diffs, prompts the user for resolution, writes | ||||||||||||||||||
| // the merged result to both the API and the local file, and returns the result. | ||||||||||||||||||
| func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, auth types.SlackAuth) (*SyncResult, error) { | ||||||||||||||||||
| 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)), | ||||||||||||||||||
| ) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
Comment on lines
+39
to
+47
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
🪓 suggestion: Am hoping the checks below can cover this validation without causing issue? 🌲 note: I lean towards keeping validation in |
||||||||||||||||||
| manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx) | ||||||||||||||||||
| if err != nil { | ||||||||||||||||||
| return nil, err | ||||||||||||||||||
|
|
@@ -77,12 +86,12 @@ func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, aut | |||||||||||||||||
|
|
||||||||||||||||||
| var merged types.AppManifest | ||||||||||||||||||
| switch { | ||||||||||||||||||
| case clients.Config.ForceFlag: | ||||||||||||||||||
| case clients.Config.ManifestSourceFlag == string(config.ManifestSourceLocal) || clients.Config.ForceFlag: | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
🪬 suggestion: Am hoping to compare objects without converting to string here. |
||||||||||||||||||
| merged, err = MergeAllFrom(localManifest.AppManifest, remoteManifest.AppManifest, diffs, MergeAllLocal) | ||||||||||||||||||
| if err != nil { | ||||||||||||||||||
| return nil, err | ||||||||||||||||||
| } | ||||||||||||||||||
| case clients.Config.ForceRemoteFlag: | ||||||||||||||||||
| case clients.Config.ManifestSourceFlag == string(config.ManifestSourceRemote) || clients.Config.ForceRemoteFlag: | ||||||||||||||||||
| merged, err = MergeAllFrom(localManifest.AppManifest, remoteManifest.AppManifest, diffs, MergeAllRemote) | ||||||||||||||||||
| if err != nil { | ||||||||||||||||||
| return nil, err | ||||||||||||||||||
|
|
@@ -91,8 +100,8 @@ func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, aut | |||||||||||||||||
| return nil, slackerror.New(slackerror.ErrAppManifestUpdate). | ||||||||||||||||||
| WithRemediation("Run %s interactively to resolve manifest differences, or pass %s to push the project manifest to app settings or %s to pull app settings to project", | ||||||||||||||||||
| style.Commandf("manifest sync", false), | ||||||||||||||||||
| style.CommandText("--force"), | ||||||||||||||||||
| style.CommandText("--force-remote"), | ||||||||||||||||||
| style.CommandText("--manifest-source=local"), | ||||||||||||||||||
| style.CommandText("--manifest-source=remote"), | ||||||||||||||||||
| ) | ||||||||||||||||||
| default: | ||||||||||||||||||
| merged, err = resolveInteractively(ctx, clients, localManifest.AppManifest, remoteManifest.AppManifest, diffs) | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
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
PreRunEflag checks but am thinking that might return both truth values with an optional error: