Feature: tools: Use formatted output in crm_diff.c #3909
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Update |
clumens
reviewed
Jul 2, 2025
nrwahl2
force-pushed
the
nrwahl2-refactors
branch
from
5717abc to
d1c477b
Compare
|
Updated to address review |
nrwahl2
force-pushed
the
nrwahl2-refactors
branch
from
d1c477b to
e50238e
Compare
|
Looks like I didn't finish the |
And limit line length of SUMMARY Signed-off-by: Reid Wahl <[email protected]>
This is highly unfortunate, but changing it would break backward compatibility. Signed-off-by: Reid Wahl <[email protected]>
It was for logging only, and it's not particularly helpful information, especially since this is invoked only from the CLI currently. Signed-off-by: Reid Wahl <[email protected]>
It's simpler to use multiple strings than to have a boolean represent whether a file name should be treated as a raw string. When we can make the input precedence sane at a compatibility break, an enum for input source will likely make the most sense. Then we can bring back callbacks to set the enum value. Also rename "apply" to "patch" for clarity, and use G_OPTION_FLAG_NONE. Use G_OPTION_ARG_NONE where appropriate. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
It doesn't copy an element, so I'd like to deprecate it. It's useful, but only slightly, and we only call it four times. Signed-off-by: Reid Wahl <[email protected]>
We also de-inline it to prevent test-headers from complaining about undefined reference to crm_element_value(). I'd like to avoid circular includes in xml_element_compat.h, and this way seems better than re-declaring crm_element_value(). Signed-off-by: Reid Wahl <[email protected]>
Not much to do here. Mainly doxygen, renaming output to patchset, a sanity check for future-proofing. Signed-off-by: Reid Wahl <[email protected]>
Sanity Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Reduce some duplication Signed-off-by: Reid Wahl <[email protected]>
Rename to check_patchset_versions() and rename arrays for clarity Signed-off-by: Reid Wahl <[email protected]>
And limit it to loop-scope. Signed-off-by: Reid Wahl <[email protected]>
Do not change behavior yet, as it will affect xml_apply_patchset(). Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
We don't do anything with input afterward, and this is static for a CLI tool, so no need to make a copy. Don't calculate a digest. It's only for tracing, and it's unclear what we would use it for. The commit that added it (2cff40d) doesn't give any explanation. Don't bother adding doxygen. We're about to drop this function. I'm doing it in two steps to clarify what it's doing and that it's no longer worth functionizing. Signed-off-by: Reid Wahl <[email protected]>
It's not really doing anything besides calling xml_apply_patchset() at this point. Signed-off-by: Reid Wahl <[email protected]>
pcmk__log_xml_patchset() already logs these, almost identically, at notice level. Signed-off-by: Reid Wahl <[email protected]>
Not really necessary but easy. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Previously, we allowed --cib and --no-version to be specified together as long as --patch was also specified. In that case, --no-version was ignored. Our option validation (or lack thereof) in crm_diff is a big mess. This is one small improvement that SHOULDN'T break anyone's workflow... if it does, they can fix it, or if it's something really insane, we can revert. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
No pcmk__message_entry_t array or pcmk__register_messages() call, because it looks like ouput_xml() will be sufficient, with no need for message functions. Signed-off-by: Reid Wahl <[email protected]>
The prompts would only make sense for text output format. They can't be sent to out->err() because it would show up as an error when the XML output is finished. If they're sent to out->info(), they won't be output at all for XML output format, and stdout can't easily be flushed for text output without checking the output format explicitly or adding new functionality to the pcmk__output_t API. We could continue printing to stderr directly, but I'd rather not. Besides, the prompts are not user-friendly at all, and I'm not sure who (if anyone) has ever used them. What they don't tell you is that you have to press Ctrl+D to mark end of input when you're done. Otherwise it will keep waiting for more input forever. I would like to eventually get rid of the --stdin option (deprecating and hiding it first), unless we can come up with a sane way to get TWO pieces of XML via stdin. Taking input from stdin makes some amount of sense when it's one piece. Signed-off-by: Reid Wahl <[email protected]>
I'm not aware of any tool in our ecosystem that uses this option. It's not user-friendly at all. Nothing tells you that you have to press Ctrl+D (on Linux) at the beginning of a line to mark the end of an XML string and indicate that you're ready to input the next one. And then press Ctrl+D again to end the second XML string. So this isn't conducive to piping input into the command, which is typically the use case for a --stdin option. There's no obvious good way to delimit the first XML input string from the second one. The prompts (removed by a previous commit) also complicated the conversion to using pcmk__output_t. Basically, there's no great way to print the prompts when using XML-formatted output, and the output isn't flushed automatically for text-formatted output, so we can't guarantee that the prompts will appear on time. We could work around this by continuing to fprintf() to stderr and then flush, but that's ugly. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
When logging an error: "ValueError: Precision not allowed in integer format specifier" Here we use width instead of precision, with zero-padding. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
It's a shame that the pcmk__output_t:output_xml() method requires preformatting the XML into a string. Since it's an internal API, maybe we can revisit that soon. Also, we already have a diff schema in xml/api. We **could** use it and output the patchset XML directly, instead of outputting the patchset XML as CDATA. Ken was pretty adamant about outputting CDATA here. I don't know why, or can't remember. The argument can't be that the XML isn't generated by crm_diff itself, because that's true for almost all of our output formatters. Something about how the format is supposed to be meaningful only to Pacemaker... though I don't see why that ought to be the case. I'm fine with CDATA, regardless. Ref T114 Signed-off-by: Reid Wahl <[email protected]>
This is pretty basic. We can have either a "patchset" element or an "updated" element, and each just contains a CDATA block. It would be possible to use the diff API schema and include the patchset XML directly rather than as CDATA. This is less straightforward to do for the "updated" XML. The input is expected to be a CIB, so I suppose we could use the CIB schema there. Unfortunately the patchset itself already uses "diff" as its root element, so I wanted to avoid using "diff" as a top-level element name for crm_diff output. It also wouldn't make a lot of sense when applying a patchset (that is, where we're displaying an "updated" element). Closes T114 Signed-off-by: Reid Wahl <[email protected]>
crm_diff --stdin is now deprecated, crm_diff now accepts --output-as=xml (and defaults to --output-as=text), and there is a new crm_diff API schema. Signed-off-by: Reid Wahl <[email protected]>
nrwahl2
force-pushed
the
nrwahl2-refactors
branch
from
e50238e to
22ae578
Compare
|
Rebased on main to resolve conflict. No other changes yet. |
clumens
reviewed
Jul 16, 2025
| @@ -38,6 +38,7 @@ | |||
|
|
|||
| #include <crm/lrmd.h> | |||
| #include <crm/cluster/internal.h> | |||
| #include <crm/common/cmdline_internal.h> | |||
There was a problem hiding this comment.
This will come as no surprise to you, but our include files are a mess. A lot of the internal includes in this list are also included by crm/common/internal.h so they could simply be removed. As far as I remember, there's no particular logic behind what is and is not included in that file.
It may be worth one of us one day doing a PR that just straightens all this stuff out. Ugh.
clumens
reviewed
Jul 16, 2025
| with open(f"{cts_cli_data}/crm_diff_old.xml", "r") as file: | ||
| old_str = f"'{file.read()}'" | ||
| with open(f"{cts_cli_data}/crm_diff_new.xml", "r") as file: | ||
| new_str = f"'{file.read()}'" |
There was a problem hiding this comment.
I might be missing something here... why can't these two just be file.read()?
|
|
||
| buffer = g_string_sized_new(1024); | ||
| pcmk__xml_string(patchset, pcmk__xml_fmt_pretty, buffer, 0); | ||
| out->output_xml(out, PCMK_XE_PATCHSET, buffer->str); |
There was a problem hiding this comment.
I'm fine with revisiting what output_xml takes as an argument. I think the reason it takes a string at the moment is likely due to whatever called it first having an XML string already. Over time, we've added more and more callers and it looks like almost all of them now have an xmlNode instead. I'm not willing to dig into the history to figure that out, so it's just a guess.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes T114