Skip to content

Commit

Permalink
fix pr comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@Enjection
Enjection committed Dec 21, 2023
1 parent bb68241 commit b6418a7
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 66 deletions.
90 changes: 38 additions & 52 deletions ydb/public/lib/ydb_cli/commands/ydb_dynamic_config.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,22 +40,17 @@ void TCommandConfigFetch::Config(TConfig& config) {
config.Opts->AddLongOption("all", "Fetch both main and volatile config")
.NoArgument().SetFlag(&All);
config.Opts->AddLongOption("output-directory", "Directory to save config(s)")
.RequiredArgument("<directory>").StoreResult(&OutDir);
.RequiredArgument("[directory]").StoreResult(&OutDir);
config.Opts->AddLongOption("strip-metadata", "Strip metadata from config")
.NoArgument().SetFlag(&StripMetadata);
config.SetFreeArgsNum(0);

config.Opts->MutuallyExclusive("all", "strip-metadata");
config.Opts->MutuallyExclusive("output-directory", "strip-metadata");
}

void TCommandConfigFetch::Parse(TConfig& config) {
TClientCommand::Parse(config);

if (All && StripMetadata) {
ythrow yexception() << "Invalid options: --all and --strip-metadata can't be used together";
}

if (OutDir && StripMetadata) {
ythrow yexception() << "Invalid options: --output-directory and --strip-metadata can't be used together";
}
}

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

if (Filename == "") {
ythrow yexception() << "Must specify -f (--filename)";
ythrow yexception() << "Must specify non-empty -f (--filename)";
}

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

DynamicConfig = configStr;

if (!IgnoreCheck) {
NYamlConfig::GetMetadata(configStr);

auto tree = NFyaml::TDocument::Parse(configStr);

auto resolved = NYamlConfig::ResolveAll(tree);

const auto resolved = NYamlConfig::ResolveAll(tree);
Y_UNUSED(resolved); // we can't check it better without ydbd
}
}
Expand Down Expand Up @@ -176,22 +168,22 @@ void TCommandConfigResolve::Config(TConfig& config) {
config.Opts->AddLongOption("all", "Resolve for all combinations")
.NoArgument().SetFlag(&All);
config.Opts->AddLongOption("label", "Labels for this node")
.Optional().RequiredArgument("<LABEL=VALUE>")
.Optional().RequiredArgument("[LABEL=VALUE]")
.KVHandler([this](TString key, TString val) {
Labels[key] = val;
});
config.Opts->AddLongOption('f', "filename", "Filename of the file containing configuration to resolve")
.Optional().RequiredArgument("<config.yaml>").StoreResult(&Filename);
.Optional().RequiredArgument("[config.yaml]").StoreResult(&Filename);
config.Opts->AddLongOption("directory", "Directory with config(s)")
.Optional().RequiredArgument("<directory>").StoreResult(&Dir);
.Optional().RequiredArgument("[directory]").StoreResult(&Dir);
config.Opts->AddLongOption("output-directory", "Directory to save config(s)")
.Optional().RequiredArgument("<directory>").StoreResult(&OutDir);
.Optional().RequiredArgument("[directory]").StoreResult(&OutDir);
config.Opts->AddLongOption("from-cluster", "Fetch current config from cluster instead of the local file")
.NoArgument().SetFlag(&FromCluster);
config.Opts->AddLongOption("remote-resolve", "Use resolver on cluster instead of built-in resolver")
.NoArgument().SetFlag(&RemoteResolve);
config.Opts->AddLongOption("node-id", "Take labels from node with the specified id")
.Optional().RequiredArgument("<node>").StoreResult(&NodeId);
.Optional().RequiredArgument("[node]").StoreResult(&NodeId);
config.Opts->AddLongOption("skip-volatile", "Ignore volatile configs")
.NoArgument().SetFlag(&SkipVolatile);
config.SetFreeArgsNum(0);
Expand All @@ -200,7 +192,7 @@ void TCommandConfigResolve::Config(TConfig& config) {
void TCommandConfigResolve::Parse(TConfig& config) {
TClientCommand::Parse(config);

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

Expand All @@ -209,14 +201,7 @@ void TCommandConfigResolve::Parse(TConfig& config) {
}
}

TString LabelSetHash(const TSet<NYamlConfig::TNamedLabel>& labels) {
TStringStream labelsStream;
labelsStream << "[";
for (const auto& label : labels) {
labelsStream << "(" << label.Name << ":" << (label.Inv ? "-" : label.Value.Quote()) << "),";
}
labelsStream << "]";
TString ser = labelsStream.Str();
TString CalcHash(const TString& ser) {
SHA_CTX ctx;
SHA1_Init(&ctx);
SHA1_Update(&ctx, ser.data(), ser.size());
Expand All @@ -228,19 +213,22 @@ TString LabelSetHash(const TSet<NYamlConfig::TNamedLabel>& labels) {
return hex;
}

TString LabelSetHash(const TSet<NYamlConfig::TNamedLabel>& labels) {
TStringStream labelsStream;
labelsStream << "[";
for (const auto& label : labels) {
labelsStream << "(" << label.Name << ":" << (label.Inv ? "-" : label.Value.Quote()) << "),";
}
labelsStream << "]";
TString ser = labelsStream.Str();
return CalcHash(ser);
}

TString ConfigHash(const NFyaml::TNodeRef& config) {
TStringStream configStream;
configStream << config;
TString ser = configStream.Str();
SHA_CTX ctx;
SHA1_Init(&ctx);
SHA1_Update(&ctx, ser.data(), ser.size());
unsigned char sha1[SHA_DIGEST_LENGTH];
SHA1_Final(sha1, &ctx);

TString hex = HexEncode(TString(reinterpret_cast<const char*>(sha1), SHA_DIGEST_LENGTH));
hex.to_lower();
return "0_" + hex;
return "0_" + CalcHash(ser);
}

int TCommandConfigResolve::Run(TConfig& config) {
Expand Down Expand Up @@ -360,7 +348,7 @@ int TCommandConfigResolve::Run(TConfig& config) {

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

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

Expand Down Expand Up @@ -394,7 +382,7 @@ int TCommandConfigResolve::Run(TConfig& config) {
configs.push_back(std::move(doc));
}
} else {
auto result = client.ResolveConfig(configStr, volatileConfigStrs, Labels).GetValueSync();
const auto result = client.ResolveConfig(configStr, volatileConfigStrs, Labels).GetValueSync();
ThrowOnError(result);

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

if (Filename.empty()) {
ythrow yexception() << "Must specify -f (--filename)";
ythrow yexception() << "Must non-empty specify -f (--filename)";
}
}

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

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

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

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

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

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

Expand Down Expand Up @@ -614,18 +602,16 @@ void TCommandConfigVolatileFetch::Config(TConfig& config) {
config.Opts->AddLongOption("all", "Fetch all volatile configs")
.NoArgument().SetFlag(&All);
config.Opts->AddLongOption("output-directory", "Directory to save config(s)")
.RequiredArgument("<directory>").StoreResult(&OutDir);
.RequiredArgument("[directory]").StoreResult(&OutDir);
config.Opts->AddLongOption("strip-metadata", "Strip metadata from config(s)")
.NoArgument().SetFlag(&StripMetadata);
config.SetFreeArgsNum(0);

config.Opts->MutuallyExclusive("output-directory", "strip-metadata");
}

void TCommandConfigVolatileFetch::Parse(TConfig& config) {
TClientCommand::Parse(config);

if (OutDir && StripMetadata) {
ythrow yexception() << "Invalid options: --output-directory and --strip-metadata can't be used together";
}
}

int TCommandConfigVolatileFetch::Run(TConfig& config) {
Expand Down
28 changes: 14 additions & 14 deletions ydb/public/lib/ydb_cli/commands/ydb_dynamic_config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ class TCommandConfigReplace : public TYdbCommand {
int Run(TConfig& config) override;

private:
bool IgnoreCheck;
bool Force;
bool DryRun;
bool AllowUnknownFields;
bool IgnoreCheck = false;
bool Force = false;
bool DryRun = false;
bool AllowUnknownFields = false;
TString DynamicConfig;
TString Filename;
};
Expand All @@ -50,13 +50,13 @@ class TCommandConfigResolve : public TYdbCommand {

private:
TMap<TString, TString> Labels;
bool All;
bool All = false;
TString Filename;
TString Dir;
TString OutDir;
bool FromCluster;
bool RemoteResolve;
bool SkipVolatile;
bool FromCluster = false;
bool RemoteResolve = false;
bool SkipVolatile = false;
ui64 NodeId;
};

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

private:
bool IgnoreCheck;
bool DryRun;
bool IgnoreCheck = false;
bool DryRun = false;
TString Filename;
};

Expand All @@ -91,8 +91,8 @@ class TCommandConfigVolatileDrop : public TYdbCommand {
THashSet<ui64> Ids;
TString Dir;
TString Filename;
bool All;
bool Force;
bool All = false;
bool Force = false;
};

class TCommandConfigVolatileFetch : public TYdbCommand {
Expand All @@ -104,9 +104,9 @@ class TCommandConfigVolatileFetch : public TYdbCommand {

private:
THashSet<ui64> Ids;
bool All;
bool All = false;
TString OutDir;
bool StripMetadata;
bool StripMetadata = false;
};

} // namespace NYdb::NConsoleClient::NDynamicConfig

0 comments on commit b6418a7

Please sign in to comment.