Skip to content
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

Enable errors from Augeas to be retrieved #1609

Open
abeverley opened this issue Oct 23, 2023 · 7 comments
Open

Enable errors from Augeas to be retrieved #1609

abeverley opened this issue Oct 23, 2023 · 7 comments
Labels
feature request A suggested idea for the project

Comments

@abeverley
Copy link
Contributor

As

an engineer

I would like to

be able to retrieve Augeas error logs

so I can

find out why Augeas commands are failing

Additional context

There is no API to the augeas "errors" command. As such, it is not possible to find out why an Augeas command is failing.

Describe the solution you would like

A new command in Rex::Commands::Augeas to be able to retrieve errors (PR to follow)

Describe alternatives you have considered

No response

@abeverley abeverley added the feature request A suggested idea for the project label Oct 23, 2023
@abeverley
Copy link
Contributor Author

I thought about automatically retrieving and adding any error information to a failing Augeas command, but I decided that is over-engineering things too much and that a developer should decide how they want to handle errors and retrieve them manually if they wish to.

abeverley added a commit to abeverley/Rex that referenced this issue Oct 23, 2023
… be retrieved

As per RexOps#1609 it is not possible to retrieve errors from Augeas if it is
failing. This PR adds a new command to be able to retrieve them.

I have not added a test, as there are currently no tests at all for the
Augeas command.

I thought about automatically retrieving and adding any error
information to a failing Augeas command, but I decided that is
over-engineering things too much and that a developer should decide how
they want to handle errors and retrieve them manually if they wish to.

Please review and merge, or let me know how to improve it further.
abeverley added a commit to abeverley/Rex that referenced this issue Oct 23, 2023
As per RexOps#1609 it is not possible to retrieve errors from Augeas if it is
failing. This PR adds a new command to be able to retrieve them.

I have not added a test, as there are currently no tests at all for the
Augeas command.

I thought about automatically retrieving and adding any error
information to a failing Augeas command, but I decided that is
over-engineering things too much and that a developer should decide how
they want to handle errors and retrieve them manually if they wish to.

Please review and merge, or let me know how to improve it further.
@ferki
Copy link
Member

ferki commented Oct 23, 2023

Thanks for opening this issue! The general idea of "being able to retrieve errors from an external system" sounds good 👍

Also thanks for the related PR! I'll take a quick look at it too, even though the main reason to open an issue first is to discuss the relevant details before investing too much into implementation.

Full disclosure: I'm not using Augeas personally, so I don't have much experience with it directly or via Rex. While I'm aware of its general scope and goals, I certainly need more pointers to understand all the details and implications of this idea.

It might be best to offload lengthier discussions for better suited support channels like GitHub discussions, Matrix/IRC, optionally in a dedicated chat as part of my open source office hours.

Here are the first few questions coming to my mind:

  1. Is the current approach the best interface we can strive for?

    What other commands are there for Augeas? Does it make sense to duplicate each command inside Rex to keeping up with each of them? Or can we have a single generic 1-to-1 direct wrapper instead (on top of which Rex may keep adding its own sugar syntax)?

    Is there a good starting resource where I could familiarize myself quickly with Augeas' own interface?

  2. Is the core the best place for this functionality?

    One might argue that Rex::Commands::Augeas would be best to split off Rex core and maintained on its own as a separate CPAN dist: Rex is pluggable, an Augeas expert may maintain it more responsibly, on their own quality terms and release cycles, etc.

    If we would decide for such split now or later, would you be interested in taking over maintenance of Rex::Commands::Augeas out of core?

  3. What exactly does the basic error checking in the current code miss?

    What's the difference between the above and the Augeas errors wished to be retrieved in the scope of this issue? Can the existing approach modified to show the errors (perhaps behind some opt-in/opt-out logic)?

  4. How it would be best to test Rex functionality related to Augeas?

    Sadly there are currently no related tests yet, which makes it hard to responsibly change related code and impossible to responsibly promise "this works now and will keep working". Therefore this change proposal is a great opportunity to make up for the lack of tests first. Or, less favorably, at least add a few initial tests only for the "errors" scope first, which may be extended later with tests for the already existing behavior.

    I imagine we can properly declare Augeas as an optional runtime dependency, like rsync and others. GitHub Actions has to install it for full tests, but otherwise the test suite may skip if Augeas is not available. We probably need some sample files to target with Augeas calls from Rex, define before/after expected states, and compare results to them. Lacking Augeas experience, I feel the most valuable contribution would be from an experienced user defining expectations for the before/after states through executable and extensible examples (this is purposely different than describing the current interface in the tests; the goal is to test the behavior, not the implementation).

@abeverley
Copy link
Contributor Author

Hi @ferki - thanks for the super-quick reply!

Thanks for all the detailed information and thoughts. I'm maxed out at the moment and so I was just quickly submitting this whilst it was causing me problems and fresh in my mind. I'll circle back round on it in more detail when I've got some more time - it looks like it could be quite a big undertaking (on the other hand, I am happy to sponsor it if anyone else wants to pick it up). I'll close the PR for the moment - sorry for the hasty opening, but at least it's there for the record.

Great to see how the project has progressed (you may remember I submitted lots of contributions several years ago!). I rely on Rex almost daily and it's a life-saver.

Many thanks,

Andy

@ferki
Copy link
Member

ferki commented Oct 25, 2023

Thanks for all the detailed information and thoughts.

In retrospect, I feel like I did not have enough time to write a shorter reply 😅 So I'm glad it still feels useful now or for future readers.

I'm maxed out at the moment and so I was just quickly submitting this whilst it was causing me problems and fresh in my mind. I'll circle back round on it in more detail when I've got some more time - it looks like it could be quite a big undertaking

Right, and it could be even bigger without tests! 😈

My view is that time is one of the most valuable resource we have, especially in a volunteer-driven project, and it is often taken from other interests and obligations such as family and self-care – so it's even more important to use it wisely and efficiently.

That's exactly the main motivation behind our Contributing guide, and emphasizing maintainability through quality.

on the other hand, I am happy to sponsor it if anyone else wants to pick it up.

That's very kind and generous of you!

In case you are looking for commercial support for Rex, including custom development or sponsoring community features, there are some more info and resources listed on our Help Rex and Support pages.

(Full disclosure: I do offer commercial Rex services as an independent provider, and listed there too. Book a chat via my calendar.)

I'll close the PR for the moment - sorry for the hasty opening, but at least it's there for the record.

No worries, it's good to have code examples available for others looking at this. It's possible to use that code as a local patch when building Rex from source. Or, since it's just Perl, put the new code into an alternative module, for example Local::Rex::Commands::Augeas or similar, and import that instead of the core one.

It might be an interesting option for next time to open PRs as draft once the scope and fitness to core is clear, and then collaborate on it until merged.

you may remember I submitted lots of contributions several years ago!

Sure I remember, and thanks again for your contributions! About half of those are for Rex::Commands::Augeas, which lead me to ask if you would be interested in maintaining a hypothetical module out of core :)

Great to see how the project has progressed [...] I rely on Rex almost daily and it's a life-saver.

Thank you for the kind words! After participating in this project for 10+ years, it feels especially great to have such feedback ❤️ I look forward to the next 10+ years too! 🚀

@abeverley
Copy link
Contributor Author

In case you are looking for commercial support for Rex, including custom development or sponsoring community features, there are some more info and resources listed on our Help Rex and Support pages.

Thank you, I will drop you a line. Whilst this particular issue is very much an edge case (it's the first time I've needed it in serveral years) I am keen to support the project to ensure its ongoing viability.

@ferki
Copy link
Member

ferki commented Sep 4, 2024

I now pushed initial Augeas tests to the test_augeas branch (see also test results and code coverage.)

This already exercises about ~50% of related code through a Simplelines use case, and could serve as a basis for further tests.

Implementing these initial tests already highlighted some hidden issues in the current code base:

  • hash order randomization makes augeas modify results unstable on Perl versions newer than 5.18.0
  • missing import in a related module
  • the Config::Augeas code paths seem unreachable
  • inconsistent augtool wrapper usage apparently leading to a augeas dump bug

I could not find a good way to test augeas insert yet – not sure that’s a bug or I’m missing some Augeas bits.

I now also better understand the Augeas wrapping use cases, and I wonder if it's best to map each augtool command to a Rex interface (like augtool errors to augeas 'errors'), or write a generic interface to support arbitrary commands (since we already have an underlying _run_augtool() to run series of augtool commands) 🤔

ps.: out of the scope of this issue, I expect the above enables the following related work too (to be tracked and discussed separetely):

  • add further tests for the above missing bits
  • add further tests for the identified bugs and fix them

@ferki
Copy link
Member

ferki commented Oct 1, 2024

Meanwhile on the test_augeas branch, I fixed most of the above bugs, included tests for both augtool and Config::Augeas backends, and fixed an error about inconsistent output between the two for dump actions.

I may still need to reorganize some of the commits there, though I think it's a good base for further tests, including the proposed augeas 'errors' action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A suggested idea for the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants