Simplify the provider interface by getting rid of suitable()

The responsibilities of suitable() and describe() were an awkward split and
it was confusing to remember which one would be called first, and which one
had to make sure the provider is set up and functional.

We now only require a describe() method that does both: set up any internal
state the provider needs before get() and set() can work, and reports
whether the provider is suitable; suitability is now stored in the provider
spec.

To narrow down how describe() finds things on the system, we now have an
environment class which provides convenience accessors to the things a
provider needs to learn about. This also helps in focusing and clarifying
what a provider writer needs to know about the libral API.

doc/invoke-native.md

@@ -2,97 +2,110 @@
 
 The C++ provider interface is defined in
 `lib/inc/libral/provider.hpp`. Note that the native interface is not
-considered stable just yet; but if you contribute a provider to the
-project, we will make sure it is updated as we make changes to the provider
-API.
+considered stable just yet; if you contribute a provider to the project, we
+will make sure it is updated as we make changes to the provider API.
 
 ## Provider interface
 
 The general interface for a provider is defined by the class
-`libral::provider` and has the following important methods:
+`libral::provider`. Specific providers need to implement three methods:
+
+* `get`: retrieves some (or all) resources that the provider can manage and
+  reports their current state
+* `set`: enforces desired state for a number of resources
+* `describe`: generates metadata about the provider and prepares the
+  provider for use
+
+Note that `libral` does not use exceptions. Instead, it uses the
+`result<T>` class which either returns a `T` value on success or an `error`
+object on failure.
 
 ```cpp
 
    // value is how libral represents attribute values
+   // the real definition of a resource is a little more complicated than
+   // this, but functions pretty much like a std::map
    using resource = std::map<std::string, value>
 
+   // update bundles an is and a should state for a resource. It has some
+   // additional convenience methods
    struct update {
-     update(const& resource is, const& resource should);
+     update(resource& is, resource& should);
 
      resource is;
      resource should;
    };
 
    class provider {
-     result<bool> suitable(runtime &rt);
-
      result<std::vector<resource>>
-     get(runtime &rt,
+     get(context &ctx,
          const& std::vector<std::string> names,
          const& std::map<std::string, value> config = { }) = 0;
 
      result<void>
-     set(runtime &rt,
+     set(context &ctx,
          const &std::vector<update> updates) = 0;
 
-     result<void> flush(runtime &rt) { };
-
-     result<void> close(runtime &rt) { };
+     result<prov::spec> describe(environment &env) { };
    };
 ```
 
 ## Provider lifecycle
 
 The lifecycle for providers written in C++ follows the outline below. This
 applies only to C++ providers - external providers follow a much simpler
-lifecycle which is described in the section above for each calling
-convention.
+lifecycle which is described in the specification of the various calling
+convention. The code below is not code you write, it's an approximation of
+what `libral` does to achieve various purposes, and is only here to
+illustrate how your provider will be used.
 
 ```cpp
-    // The runtime object provides helpers for providers that they can use
-    // to interact with their environment.
-    libral::runtime rt;
-
+    // The context and environment objects provide helpers and should be
+    // the sole APIs that providers use to call back into libral
+    libral::context ctx;
+    libral::environment env;
+
+    // Because reporting errors from constructors is so awkward, provider
+    // constructors should do as little work as possible. Anything that
+    // could fail should be done in the describe() method so that it can be
+    // reported back properly
     some_provider prov();
 
-    // A call to suitable() must also initialize any provider internals.
-    // Once suitable() returns true, the provider must be ready to use.
-    // The suitable method most call rt.register(metadata) where metadata
-    // is the provider metadata
-    if (prov.suitable(rt)) {
+    // A call to describe() must return metadata about the provider and
+    // also initialize any provider internals. In particular, it must
+    // indicate whether the provider is suitable or not. That means that
+    // describe must check whether all its dependencies are met (e.g., that
+    // all commands the provider wants to use are available)
+    auto spec = prov.describe().ok();
+
+    if (spec.suitable()) {
       // List all resources
-      auto all = prov.get(rt, { });
+      auto all = prov.get(ctx, { });
 
       // Find one resource
-      auto one = prov.get(rt, { "name" });
+      auto one = prov.get(ctx, { "name" });
 
       // Find a few resources
-      auto some = prov.get(rt, { "name1", "name2", "name3" });
+      auto some = prov.get(ctx, { "name1", "name2", "name3" });
 
       // Find a resource with additional provider configuration
-      auto one_conf = prov.get(rt, { "name" }, { "target", "/etc/my_fstab" });
+      auto one_conf = prov.get(ctx, { "name" }, { "target", "/etc/my_fstab" });
 
       // do something to a specific resource
-      auto rsrc = prov.get(rt, { "some_name" });
-      attr_map should = { { "ensure", "absent" } };
-      prov.set(rt, { update(rsrc, should) });
-
-      // create a new one
-      auto creat = get_new_attrs_from_somewhere_else();
-      prov.set(rt, { update(nullptr, creat) });
+      auto is = prov.get(ctx, { "some_name" })->front();
+      auto should = prov.create(is.name());
+      should["ensure"] = "absent";
 
-      // make sure all changes have been written to disk
-      prov.flush(rt);
+      prov.set(ctx, { update(is, should) });
 
-      // not sure yet if we need an explicit 'close' call
-      prov.close(rt)
+      // create a new one
+      // get must return a resource with 'ensure' == 'absent' if the
+      // resource does not exist yet, but could be created
+      auto is = prov.get(ctx, { "some_name" })->front();
+      auto should = prov.create(is.name());
+      should["ensure"] = "present";
+      should[...] = ...; // set more attributes
+
+      prov.set(ctx, { update(is, should) });
     }
 ```
-
-## Runtime interface
-
-```cpp
-
-    class runtime {
-
-    };

lib/CMakeLists.txt

@@ -34,7 +34,8 @@ set(PROJECT_SOURCES "src/libral.cc" "src/augeas/handle.cc" "src/augeas/node.cc"
   "src/simple_provider.cc" "src/json_provider.cc"
   "src/user.cc" "src/value.cc" "src/file.cc"
   "src/prov/spec.cc" "src/attr/spec.cc"
-  "src/command.cc" "src/resource.cc" "src/context.cc")
+  "src/command.cc" "src/resource.cc" "src/context.cc"
+  "src/environment.cc")
 
 ## An object target is generated that can be used by both the library and test executable targets.
 ## Without the intermediate target, unexported symbols can't be tested.

lib/inc/libral/environment.hpp

@@ -0,0 +1,65 @@
+#pragma once
+
+#include <vector>
+
+#include <boost/optional.hpp>
+
+#include <libral/command.hpp>
+#include <libral/augeas.hpp>
+#include <libral/prov/spec.hpp>
+
+namespace libral {
+  // Forward declaration to break an include loop
+  class ral;
+
+  /**
+   * This class represents the environment in which providers operate and
+   * contains convenience function for accessing various aspects of the
+   * environment. An instance of this class is passed to the provider's
+   * describe method. */
+  class environment {
+  public:
+    /**
+     * Looks for a command called \p cmd and returns a command object that
+     * can be used to run the command.
+     *
+     * @return \c boost::none if no command \p cmd can be found, or a
+     * command object to run this command.
+     */
+    boost::optional<libral::command> command(const std::string& cmd);
+
+    /**
+     * Creates an augeas handle that has the files described by \p xfms
+     * loaded.
+     */
+    result<std::shared_ptr<augeas::handle>>
+    augeas(const std::vector<std::pair<std::string, std::string>>& xfms);
+
+    const std::vector<std::string>& data_dirs() const;
+
+    /**
+     * Creates provider metadata from the description \p desc, which must
+     * be valid YAML and comply with the provider metadata
+     * specification. The \p name is used as the default name for the
+     * provider, unless the metadata specifies a different name.
+     *
+     * If \p suitable is not \c boost::none, it takes precedence over a
+     * suitable entry that might be in \p desc. If \p suitable is
+     * boost::none, then \p desc must contain an entry that indicates
+     * whether the provider is suitable or not.
+     */
+    result<prov::spec> parse_spec(const std::string& name,
+                                  const std::string& desc,
+                                  boost::optional<bool> suitable = boost::none);
+
+    result<prov::spec>
+    parse_spec(const std::string& name, const YAML::Node &node);
+
+  protected:
+    friend class ral;
+
+    environment(std::shared_ptr<ral> ral) : _ral(ral) { }
+  private:
+    std::shared_ptr<ral> _ral;
+  };
+}

lib/inc/libral/file.hpp

@@ -58,8 +58,6 @@ namespace libral {
    */
   class file_provider : public provider {
   public:
-    result<bool> suitable() override { return true; };
-
     result<std::vector<resource>>
     get(context &ctx,
         const std::vector<std::string>& names,
@@ -68,7 +66,7 @@ namespace libral {
     result<void> set(context &ctx, const updates& upds) override;
 
   protected:
-    result<prov::spec> describe() override;
+    result<prov::spec> describe(environment &env) override;
   private:
     result<void> set(context &ctx, const update& upd);
 

lib/inc/libral/json_provider.hpp

@@ -14,8 +14,6 @@ namespace libral {
     json_provider(const std::string& path, YAML::Node &node)
       : provider(), _path(path), _node(node) { };
 
-    result<bool> suitable() override;
-
     result<std::vector<resource>>
     get(context& ctx, const std::vector<std::string>& names,
         const resource::attributes& config) override;
@@ -24,7 +22,7 @@ namespace libral {
 
     const std::string& source() const override { return _path; }
   protected:
-    result<prov::spec> describe() override;
+    result<prov::spec> describe(environment &env) override;
   private:
     result<void> set(context &ctx, const update &upd);
 

lib/inc/libral/mount.hpp

@@ -34,10 +34,7 @@ namespace libral {
 
   class mount_provider : public provider {
   public:
-    mount_provider(std::shared_ptr<ral> ral)
-      : _aug(nullptr), _ral(ral), _seq(1) { };
-
-    result<bool> suitable() override;
+    mount_provider() : _aug(nullptr), _seq(1) { };
 
     result<std::vector<resource>>
     get(context &ctx, const std::vector<std::string>& names,
@@ -46,7 +43,8 @@ namespace libral {
     result<void> set(context &ctx, const updates& upds) override;
 
   protected:
-    result<prov::spec> describe() override;
+    result<prov::spec> describe(environment& env) override;
+
   private:
     augeas::node base(const update &upd);
     result<resource> make(const std::string& name,
@@ -62,7 +60,6 @@ namespace libral {
     std::shared_ptr<augeas::handle> _aug;
     boost::optional<command>     _cmd_mount;
     boost::optional<command>     _cmd_umount;
-    std::shared_ptr<ral>         _ral;
     // We use this to create new paths in the augeas tree when creating entries
     int                          _seq;
   };

lib/inc/libral/prov/spec.hpp

@@ -36,6 +36,14 @@ namespace libral { namespace prov {
      */
     const std::string& desc() const {return _desc; }
 
+    /**
+     * Returns true if the provider is suitable, i.e., can be used
+     * successfully on this system
+     */
+    bool suitable() const { return _suitable; }
+
+    void suitable(bool s) { _suitable = s; }
+
     /**
      * Reads a provider specification from a parsed YAML representation
      *
@@ -66,6 +74,7 @@ namespace libral { namespace prov {
     std::string   _type;
     std::string   _desc;
     std::string   _qname;
+    bool          _suitable;
 
     attr_spec_map _attr_specs;
   };

lib/inc/libral/provider.hpp

@@ -2,6 +2,7 @@
 
 #include <vector>
 #include <map>
+#include <memory>
 
 #include <boost/optional.hpp>
 #include <boost/variant.hpp>
@@ -10,6 +11,7 @@
 #include <libral/value.hpp>
 #include <libral/resource.hpp>
 #include <libral/context.hpp>
+#include <libral/environment.hpp>
 #include <libral/prov/spec.hpp>
 
 namespace libral {
@@ -29,15 +31,6 @@ namespace libral {
     provider() { };
     virtual ~provider() = default;
 
-    /**
-     * Returns true if the provider can be used on the system. Returns
-     * false if the provider can not be used on the system.
-     *
-     * If a problem is encountered that should be considered an error and
-     * reported back to the user, return an error result. The provider will
-     * be considered not suitable in that case.  */
-    virtual result<bool> suitable() = 0;
-
     /**
      * Retrieve the resources managed by this provider. At least the
      * resources mentioned in NAMES must be included in the returned
@@ -96,14 +89,18 @@ namespace libral {
     friend class ral;
 
     /**
-     * Sets up internal data structures and is called after suitable() is
+     * Gets the provider ready for use. Calls describe() to kick off any
+     * provider-specific initialization.
      */
-    result<bool> prepare();
+    result<void> prepare(environment &env);
 
     /**
-     * Returns the provider spec for this provider.
+     * Constructs and returns the provider spec. The \p env object can be
+     * used to interact with the runtime environment. This method must also
+     * do any provider-internal initialization and check whether the
+     * provider is suitable for this system.
      */
-    virtual result<prov::spec> describe() = 0;
+    virtual result<prov::spec> describe(environment &env) = 0;
   private:
     /* This gets only intitialized when we call prepare, and we therefore
      * must allow for it to be none for a while