Skip to content

Commit b6418a7

Browse files
EnjectionEnjection
committed
fix pr comments
1 parent bb68241 commit b6418a7

File tree

2 files changed

+52
-66
lines changed

2 files changed

+52
-66
lines changed

‎ydb/public/lib/ydb_cli/commands/ydb_dynamic_config.cpp‎

Lines changed: 38 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -40,22 +40,17 @@ void TCommandConfigFetch::Config(TConfig& config) {
4040
config.Opts->AddLongOption("all", "Fetch both main and volatile config")
4141
.NoArgument().SetFlag(&All);
4242
config.Opts->AddLongOption("output-directory", "Directory to save config(s)")
43-
.RequiredArgument("<directory>").StoreResult(&OutDir);
43+
.RequiredArgument("[directory]").StoreResult(&OutDir);
4444
config.Opts->AddLongOption("strip-metadata", "Strip metadata from config")
4545
.NoArgument().SetFlag(&StripMetadata);
4646
config.SetFreeArgsNum(0);
47+
48+
config.Opts->MutuallyExclusive("all", "strip-metadata");
49+
config.Opts->MutuallyExclusive("output-directory", "strip-metadata");
4750
}
4851

4952
void TCommandConfigFetch::Parse(TConfig& config) {
5053
TClientCommand::Parse(config);
51-
52-
if (All && StripMetadata) {
53-
ythrow yexception() << "Invalid options: --all and --strip-metadata can't be used together";
54-
}
55-
56-
if (OutDir && StripMetadata) {
57-
ythrow yexception() << "Invalid options: --output-directory and --strip-metadata can't be used together";
58-
}
5954
}
6055

6156
int TCommandConfigFetch::Run(TConfig& config) {
@@ -112,7 +107,7 @@ TCommandConfigReplace::TCommandConfigReplace()
112107
void TCommandConfigReplace::Config(TConfig& config) {
113108
TYdbCommand::Config(config);
114109
config.Opts->AddLongOption('f', "filename", "Filename of the file containing configuration")
115-
.Optional().RequiredArgument("<config.yaml>").StoreResult(&Filename);
110+
.Required().RequiredArgument("[config.yaml]").StoreResult(&Filename);
116111
config.Opts->AddLongOption("ignore-local-validation", "Ignore local config applicability checks")
117112
.NoArgument().SetFlag(&IgnoreCheck);
118113
config.Opts->AddLongOption("dry-run", "Check config applicability")
@@ -128,20 +123,17 @@ void TCommandConfigReplace::Parse(TConfig& config) {
128123
TClientCommand::Parse(config);
129124

130125
if (Filename == "") {
131-
ythrow yexception() << "Must specify -f (--filename)";
126+
ythrow yexception() << "Must specify non-empty -f (--filename)";
132127
}
133128

134-
auto configStr = Filename == "-" ? Cin.ReadAll() : TFileInput(Filename).ReadAll();
129+
const auto configStr = Filename == "-" ? Cin.ReadAll() : TFileInput(Filename).ReadAll();
135130

136131
DynamicConfig = configStr;
137132

138133
if (!IgnoreCheck) {
139134
NYamlConfig::GetMetadata(configStr);
140-
141135
auto tree = NFyaml::TDocument::Parse(configStr);
142-
143-
auto resolved = NYamlConfig::ResolveAll(tree);
144-
136+
const auto resolved = NYamlConfig::ResolveAll(tree);
145137
Y_UNUSED(resolved); // we can't check it better without ydbd
146138
}
147139
}
@@ -176,22 +168,22 @@ void TCommandConfigResolve::Config(TConfig& config) {
176168
config.Opts->AddLongOption("all", "Resolve for all combinations")
177169
.NoArgument().SetFlag(&All);
178170
config.Opts->AddLongOption("label", "Labels for this node")
179-
.Optional().RequiredArgument("<LABEL=VALUE>")
171+
.Optional().RequiredArgument("[LABEL=VALUE]")
180172
.KVHandler([this](TString key, TString val) {
181173
Labels[key] = val;
182174
});
183175
config.Opts->AddLongOption('f', "filename", "Filename of the file containing configuration to resolve")
184-
.Optional().RequiredArgument("<config.yaml>").StoreResult(&Filename);
176+
.Optional().RequiredArgument("[config.yaml]").StoreResult(&Filename);
185177
config.Opts->AddLongOption("directory", "Directory with config(s)")
186-
.Optional().RequiredArgument("<directory>").StoreResult(&Dir);
178+
.Optional().RequiredArgument("[directory]").StoreResult(&Dir);
187179
config.Opts->AddLongOption("output-directory", "Directory to save config(s)")
188-
.Optional().RequiredArgument("<directory>").StoreResult(&OutDir);
180+
.Optional().RequiredArgument("[directory]").StoreResult(&OutDir);
189181
config.Opts->AddLongOption("from-cluster", "Fetch current config from cluster instead of the local file")
190182
.NoArgument().SetFlag(&FromCluster);
191183
config.Opts->AddLongOption("remote-resolve", "Use resolver on cluster instead of built-in resolver")
192184
.NoArgument().SetFlag(&RemoteResolve);
193185
config.Opts->AddLongOption("node-id", "Take labels from node with the specified id")
194-
.Optional().RequiredArgument("<node>").StoreResult(&NodeId);
186+
.Optional().RequiredArgument("[node]").StoreResult(&NodeId);
195187
config.Opts->AddLongOption("skip-volatile", "Ignore volatile configs")
196188
.NoArgument().SetFlag(&SkipVolatile);
197189
config.SetFreeArgsNum(0);
@@ -200,7 +192,7 @@ void TCommandConfigResolve::Config(TConfig& config) {
200192
void TCommandConfigResolve::Parse(TConfig& config) {
201193
TClientCommand::Parse(config);
202194

203-
if (((ui32)(!Filename.empty()) + (ui32)(!Dir.empty()) + (ui32)FromCluster) != 1) {
195+
if ((ui32(!Filename.empty()) + ui32(!Dir.empty()) + ui32(FromCluster)) != 1) {
204196
ythrow yexception() << "Must specify one of -f (--filename), --directory and --from-cluster";
205197
}
206198

@@ -209,14 +201,7 @@ void TCommandConfigResolve::Parse(TConfig& config) {
209201
}
210202
}
211203

212-
TString LabelSetHash(const TSet<NYamlConfig::TNamedLabel>& labels) {
213-
TStringStream labelsStream;
214-
labelsStream << "[";
215-
for (const auto& label : labels) {
216-
labelsStream << "(" << label.Name << ":" << (label.Inv ? "-" : label.Value.Quote()) << "),";
217-
}
218-
labelsStream << "]";
219-
TString ser = labelsStream.Str();
204+
TString CalcHash(const TString& ser) {
220205
SHA_CTX ctx;
221206
SHA1_Init(&ctx);
222207
SHA1_Update(&ctx, ser.data(), ser.size());
@@ -228,19 +213,22 @@ TString LabelSetHash(const TSet<NYamlConfig::TNamedLabel>& labels) {
228213
return hex;
229214
}
230215

216+
TString LabelSetHash(const TSet<NYamlConfig::TNamedLabel>& labels) {
217+
TStringStream labelsStream;
218+
labelsStream << "[";
219+
for (const auto& label : labels) {
220+
labelsStream << "(" << label.Name << ":" << (label.Inv ? "-" : label.Value.Quote()) << "),";
221+
}
222+
labelsStream << "]";
223+
TString ser = labelsStream.Str();
224+
return CalcHash(ser);
225+
}
226+
231227
TString ConfigHash(const NFyaml::TNodeRef& config) {
232228
TStringStream configStream;
233229
configStream << config;
234230
TString ser = configStream.Str();
235-
SHA_CTX ctx;
236-
SHA1_Init(&ctx);
237-
SHA1_Update(&ctx, ser.data(), ser.size());
238-
unsigned char sha1[SHA_DIGEST_LENGTH];
239-
SHA1_Final(sha1, &ctx);
240-
241-
TString hex = HexEncode(TString(reinterpret_cast<const char*>(sha1), SHA_DIGEST_LENGTH));
242-
hex.to_lower();
243-
return "0_" + hex;
231+
return "0_" + CalcHash(ser);
244232
}
245233

246234
int TCommandConfigResolve::Run(TConfig& config) {
@@ -360,7 +348,7 @@ int TCommandConfigResolve::Run(TConfig& config) {
360348

361349
TVector<TString> labels(result.GetLabels().begin(), result.GetLabels().end());
362350

363-
for (auto& [labelSets, configStr] : result.GetConfigs()) {
351+
for (const auto& [labelSets, configStr] : result.GetConfigs()) {
364352
auto doc = NFyaml::TDocument::Parse("---\nlabel_sets: []\nconfig: {}\n");
365353
auto config = NFyaml::TDocument::Parse(configStr);
366354

@@ -394,7 +382,7 @@ int TCommandConfigResolve::Run(TConfig& config) {
394382
configs.push_back(std::move(doc));
395383
}
396384
} else {
397-
auto result = client.ResolveConfig(configStr, volatileConfigStrs, Labels).GetValueSync();
385+
const auto result = client.ResolveConfig(configStr, volatileConfigStrs, Labels).GetValueSync();
398386
ThrowOnError(result);
399387

400388
auto doc = NFyaml::TDocument::Parse("---\nlabel_sets: []\nconfig: {}\n");
@@ -453,7 +441,7 @@ TCommandConfigVolatileAdd::TCommandConfigVolatileAdd()
453441
void TCommandConfigVolatileAdd::Config(TConfig& config) {
454442
TYdbCommand::Config(config);
455443
config.Opts->AddLongOption('f', "filename", "filename to set")
456-
.Optional().RequiredArgument("<config.yaml>").StoreResult(&Filename);
444+
.Required().RequiredArgument("[config.yaml]").StoreResult(&Filename);
457445
config.Opts->AddLongOption("ignore-local-validation", "Ignore local config applicability checks")
458446
.NoArgument().SetFlag(&IgnoreCheck);
459447
config.Opts->AddLongOption("dry-run", "Check config applicability")
@@ -466,15 +454,15 @@ void TCommandConfigVolatileAdd::Parse(TConfig& config) {
466454
TClientCommand::Parse(config);
467455

468456
if (Filename.empty()) {
469-
ythrow yexception() << "Must specify -f (--filename)";
457+
ythrow yexception() << "Must non-empty specify -f (--filename)";
470458
}
471459
}
472460

473461
int TCommandConfigVolatileAdd::Run(TConfig& config) {
474462
auto driver = std::make_unique<NYdb::TDriver>(CreateDriver(config));
475463
auto client = NYdb::NDynamicConfig::TDynamicConfigClient(*driver);
476464

477-
auto configStr = Filename == "-" ? Cin.ReadAll() : TFileInput(Filename).ReadAll();
465+
const auto configStr = Filename == "-" ? Cin.ReadAll() : TFileInput(Filename).ReadAll();
478466

479467
if (!IgnoreCheck) {
480468
auto result = client.GetConfig().GetValueSync();
@@ -526,17 +514,17 @@ void TCommandConfigVolatileDrop::Config(TConfig& config) {
526514
config.Opts->AddLongOption("force", "Ignore version and cluster check")
527515
.NoArgument().SetFlag(&Force);
528516
config.Opts->AddLongOption("directory", "Directory with volatile configs")
529-
.Optional().RequiredArgument("<directory>").StoreResult(&Dir);
517+
.Optional().RequiredArgument("[directory]").StoreResult(&Dir);
530518
}
531519

532520
void TCommandConfigVolatileDrop::Parse(TConfig& config) {
533521
TClientCommand::Parse(config);
534522

535-
if (((ui32)!Ids.empty() + (ui32)All + (ui32)!Filename.empty() + (ui32)!Dir.empty()) != 1) {
523+
if ((ui32(!Ids.empty()) + ui32(All) + ui32(!Filename.empty()) + ui32(!Dir.empty())) != 1) {
536524
ythrow yexception() << "Must specify one of --id, --all, -f (--filename) and --directory";
537525
}
538526

539-
if ((ui32)(!Filename.empty() || !Dir.empty()) + (ui32)(!Ids.empty() || All) != 1) {
527+
if ((ui32(!Filename.empty() || !Dir.empty()) + ui32(!Ids.empty() || All)) != 1) {
540528
ythrow yexception() << "Must specify either --directory or -f (--filename) and --id or --all";
541529
}
542530

@@ -614,18 +602,16 @@ void TCommandConfigVolatileFetch::Config(TConfig& config) {
614602
config.Opts->AddLongOption("all", "Fetch all volatile configs")
615603
.NoArgument().SetFlag(&All);
616604
config.Opts->AddLongOption("output-directory", "Directory to save config(s)")
617-
.RequiredArgument("<directory>").StoreResult(&OutDir);
605+
.RequiredArgument("[directory]").StoreResult(&OutDir);
618606
config.Opts->AddLongOption("strip-metadata", "Strip metadata from config(s)")
619607
.NoArgument().SetFlag(&StripMetadata);
620608
config.SetFreeArgsNum(0);
609+
610+
config.Opts->MutuallyExclusive("output-directory", "strip-metadata");
621611
}
622612

623613
void TCommandConfigVolatileFetch::Parse(TConfig& config) {
624614
TClientCommand::Parse(config);
625-
626-
if (OutDir && StripMetadata) {
627-
ythrow yexception() << "Invalid options: --output-directory and --strip-metadata can't be used together";
628-
}
629615
}
630616

631617
int TCommandConfigVolatileFetch::Run(TConfig& config) {

‎ydb/public/lib/ydb_cli/commands/ydb_dynamic_config.h‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,10 @@ class TCommandConfigReplace : public TYdbCommand {
2020
int Run(TConfig& config) override;
2121

2222
private:
23-
bool IgnoreCheck;
24-
bool Force;
25-
bool DryRun;
26-
bool AllowUnknownFields;
23+
bool IgnoreCheck = false;
24+
bool Force = false;
25+
bool DryRun = false;
26+
bool AllowUnknownFields = false;
2727
TString DynamicConfig;
2828
TString Filename;
2929
};
@@ -50,13 +50,13 @@ class TCommandConfigResolve : public TYdbCommand {
5050

5151
private:
5252
TMap<TString, TString> Labels;
53-
bool All;
53+
bool All = false;
5454
TString Filename;
5555
TString Dir;
5656
TString OutDir;
57-
bool FromCluster;
58-
bool RemoteResolve;
59-
bool SkipVolatile;
57+
bool FromCluster = false;
58+
bool RemoteResolve = false;
59+
bool SkipVolatile = false;
6060
ui64 NodeId;
6161
};
6262

@@ -73,8 +73,8 @@ class TCommandConfigVolatileAdd : public TYdbCommand {
7373
int Run(TConfig& config) override;
7474

7575
private:
76-
bool IgnoreCheck;
77-
bool DryRun;
76+
bool IgnoreCheck = false;
77+
bool DryRun = false;
7878
TString Filename;
7979
};
8080

@@ -91,8 +91,8 @@ class TCommandConfigVolatileDrop : public TYdbCommand {
9191
THashSet<ui64> Ids;
9292
TString Dir;
9393
TString Filename;
94-
bool All;
95-
bool Force;
94+
bool All = false;
95+
bool Force = false;
9696
};
9797

9898
class TCommandConfigVolatileFetch : public TYdbCommand {
@@ -104,9 +104,9 @@ class TCommandConfigVolatileFetch : public TYdbCommand {
104104

105105
private:
106106
THashSet<ui64> Ids;
107-
bool All;
107+
bool All = false;
108108
TString OutDir;
109-
bool StripMetadata;
109+
bool StripMetadata = false;
110110
};
111111

112112
} // namespace NYdb::NConsoleClient::NDynamicConfig

0 commit comments

Comments
 (0)