-
Notifications
You must be signed in to change notification settings - Fork 12
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
pack: skip unavailable configs from etcd and tcs #1066
pack: skip unavailable configs from etcd and tcs #1066
Conversation
ddf6386
to
c75c205
Compare
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.
Please, add an entry into CHANGELOG.md.
cli/pack/opts.go
Outdated
@@ -49,7 +49,7 @@ func initAppsInfo(cliOpts *config.CliOpts, cmdCtx *cmdcontext.CmdCtx, packCtx *P | |||
} | |||
packCtx.AppList = appList | |||
packCtx.AppsInfo, err = running.CollectInstancesForApps(packCtx.AppList, cliOpts, | |||
cmdCtx.Cli.ConfigDir, cmdCtx.Integrity, true) | |||
cmdCtx.Cli.ConfigDir, cmdCtx.Integrity, false) |
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.
It looks like the same approach is to be applied to both instances scripts and cluster config. So both of them are required or both of them are optional and it seems wrong in case of tt pack
. I'm not sure about cluster config, maybe it's correct that we treat it as configuration and thus able to drop from package, but it seems we are not able to treat instances scripts in the same way because they contain business logic and thus look like the essential part of the package.
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.
And regardless of our ability to drop some part of package while preparing it, it looks quite weird that content of the package depends on current availability of some items. What if next time one pack the same application but etcd/tcs config is available? As the content is changed we will get another package for the same application. Do we expect such a behavior?
c75c205
to
7b9cd77
Compare
30ff7e3
to
b64c71a
Compare
b64c71a
to
bfb7321
Compare
bfb7321
to
460b4fa
Compare
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.
We still not skip a config load, but load a config and skip an error.
bcefd0a
to
f729a21
Compare
f729a21
to
48559b6
Compare
48559b6
to
bf7ba6c
Compare
4bb9df1
to
cd71f6f
Compare
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.
You still tries to load the config even ConfigLoadSkip == true.
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.
Ok, let's merge it now. And left the proper code for the future refactoring. Please, update tests (see a comment above).
41dc8f2
to
6c0c5f6
Compare
Please, rebase on the master branch to fix the CI. |
e606a21
to
808a902
Compare
@TarantoolBot document Title: pack: skip configs from etcd and tcs This patch fixes the `tt pack` error if `etcd` or `tcs` are unavailable during package creation. Part of #1038
808a902
to
3328b6e
Compare
@@ -126,6 +126,21 @@ type providerImpl struct { | |||
instanceCtx *InstanceCtx | |||
} | |||
|
|||
type ConfigLoad int |
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.
Comment missed.
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.
Thanks for the patch! LGTM.
From now on `build`, `completion`, `kill`, `log`, `logrotate`, `stop`, `list`, `pack` commands skip instance config generation from local, etcs/tcs and env sources. Closes #1038
3328b6e
to
0845d96
Compare
Closes #1038
@TarantoolBot document
Title: pack: skip unavailable configs from etcd and tcs
This patch fixes the
tt pack
error ifetcd
ortcs
are unavailable during package creation.