WIP: Repository class refactoring #95

arthurmco · 2025-01-27T14:11:03Z

Refactor the repository class based on @dhilst 's modeling

Edit

Context:

I (Daniel) generalized RepoManager over the repository type and the runner,
Because of that I generalized Cluster over the runner too, this is intentional for us to track who is executing code through clusters's runner
The Cluster's new parameter propagated through the code base, I used BaseRunner in most places where it doesn't matter, and added a type parameter where it does (where it is getting the runner or the repo manager)

TODO:

Replace the old implementation of RepoManager with the new implementation
Remove the dependency Runner from Cluster, if something is depending on the cluster's runner to run commands we'll find out, this something should be running commands on its own

Edit

More context:

I added namespaces cloyster::models and cloyster::services to start splitting the code according to the model-view-presenter architecture
I moved the files so they correspond to their namespaces
I'm looking into the RepoManager logic now and how the clients use it
I added cloyster::services::files::KeyFile class to encapsulate the glibmm implementation of keyfile, we must use it without leaking implementation details (glib types)

viniciusferrao · 2025-01-27T14:19:24Z

src/services/repo.cpp

+        return file;
+    } catch (Glib::FileError& e) {
+        throw repository_exception(
+            std::format("Could not load repository file {} ({})",


fmt::format.

viniciusferrao · 2025-01-27T14:19:39Z

src/services/repo.cpp

+
+void ELRepoFile::read()
+{
+    printf("reading\n");


viniciusferrao · 2025-01-27T14:20:17Z

src/services/repo.cpp

+    for (const auto& r : m_repositories) {
+        std::print("repo found '{}'\n", r.group);
+        std::print("\tname: '{}'\n", r.name);
+        std::print("\tbaseurl: '{}'\n", r.baseURL.value_or("null"));
+        std::print("\tmetalink: '{}'\n", r.metalink.value_or("null"));
+        std::print("\tenabled: {}\n", r.enabled);
+        std::print("\tgpg: (check: {}, key: {})\n", r.gpgcheck, r.gpgkey);
+    }


Is this debug? If yes add #ifndef NDEBUG. Also we are using fmt::format instead and use the LOG_DEBUG directive.

This was glibmm test code I forgot to remove.

include/cloysterhpc/services/repo.h

viniciusferrao · 2025-01-27T14:25:50Z

src/services/repo.cpp

+        m_repositories.push_back(std::move(repo));
+    }
+
+    for (const auto& r : m_repositories) {


I would avoid using only r as variable name. Usually we use it for well defined counters, like: i, j and k. Or for iterators, like it.

This is only code for toying with glibmm

In real life, this would not be like this

(This comment applies to every printf/fmt::format comment above)

viniciusferrao · 2025-01-27T14:27:43Z

Refactor the repository class based on @dhilst 's modeling

The modeling was done by the three of us. You included!

dhilst · 2025-01-27T14:30:13Z

include/cloysterhpc/services/repo.h

+/**
+ * Repository file class
+ */
+template <class Repo> class RepoFile {
+protected:
+    std::filesystem::path m_path;
+    std::vector<Repo> m_repositories;


You are mixing up "repository" and File class

Remove the Repo type parameter

Rename RepoFile to File

Remove m_repositories member

This class only wraps file I/O, it must not know anything about repositories

Like this?

class File { protected: std::filesystem::path m_path; public: RepoFile(const std::filesystem::path& path) : m_path(path) {} virtual void read() {} virtual void write() {} };

(Me including the repositories list there was a mistake... I already removed it)

Partially, there will be an entity named something like "File" on the namespace "cloysterhpc" that will handle basic file operations. Open, close, write, read, chmod. There's a lot of that on glib and we may use them:

https://gnome.pages.gitlab.gnome.org/glibmm/classGio_1_1File.html
https://gnome.pages.gitlab.gnome.org/glibmm/group__FileUtils.html
https://gnome.pages.gitlab.gnome.org/glibmm/group__UriUtils.html

I do believe that GIO will solve almost everything.

dhilst · 2025-01-27T14:34:15Z

include/cloysterhpc/services/repo.h

+class ELRepoFile : public RepoFile<ELRepo> {
+private:
+    Glib::RefPtr<Glib::KeyFile> loadFile();
+
+public:
+    ELRepoFile(const std::filesystem::path& path)
+        : RepoFile(path)
+    {
+    }
+
+    void read() override;
+    void write() override;
+};


We can have an abstract RepoFile which extends the File, adds the m_repositories generic member, and with parse unparse methods, read is super read followed by parse and write is unparse followed by super write

std::filesystem::path is convertible to std::string or const char*. We must use it. It's the right API.

However the file class that be dependent on std::filesystem::path and not the derived one.

Just to be clear

parse loads data from the file into the m_repositories member

unparse loads the m_repositories member into the file

Also, @arthurmco, add overloads to parse and unparse which I/O into istringstream ostringstream instead of directly into the file so we can test without touching the disks. I don't know if this is possible because I don't know the interface of glibmm if it can load/unload keyfiles from strings, can it?

This is possible; glibmm can load keyhole data from a string.

include/cloysterhpc/services/repo.h

dhilst · 2025-01-27T17:03:02Z

src/services/repo.cpp

+        auto metalink = (file->has_key(repogroup, "metalink"))
+            ? std::make_optional(file->get_string(repogroup, "metalink"))
+            : std::nullopt;


Optionals are really cool. Add a free function for reading an optional from keyfiles

I'm an idiot. I have too much difficulty reading ternary operators. That's a technical debt in my mind.

The std::nullopt should be inside a std::make_optional? I don't know.

It is okay, you can read this like this if (file->has_key(repogroup, "metalink")) { metalink = std::make_optional(file->get_string(repogroup, "metalink")) } else { metalink = std::nullopt; }. metalink is an optional set to the metalink property if it exists in the file, or set to nullopt if not. This is pretty general, we'll want to use std::optional to represent optional values in the conf file, so I'm suggesting that we create a function for that

inline std::optional<std::string> get_optional_string( const KeyFile& fil, const std::string& group, const std::string& key) { if (fil->has_key(group, key)) { return fil->get_string(group, key); } return std::nullopt; }

The optional is got with the .value() accessor, right?

yes, but glib keyfile is not using optional here, I guess the interface is older than std::optional

yes, but glib keyfile is not using optional here, I guess the interface is older than std::optional

Yes it is. But we are talking about the same issue? That our API shuould provide access with std::optional and inside the class we encapsulate that? If that's the case yes I do agree.

Are you saying for us to extend glibmm file interface and add this as a member function?

dhilst · 2025-01-30T13:48:31Z

src/services/repo.cpp

+std::vector<ELRepo> ELRepoFile::parse(const std::stringstream& ss)
+{
+    m_file = Glib::KeyFile::create();
+    m_file->load_from_data(ss.str().c_str());
+    return this->parseData();
+}


Cool, now

Add m_repositories member as a vector of ELRepo with a getter

The getter should load the data into the member if the member is empty and return at the end

Inside of ELRepo?

Inside ELRepoFile

dhilst · 2025-01-30T13:50:52Z

include/cloysterhpc/services/repo.h

+struct ELRepo {
+    std::string group;
+    std::string name;
+    std::optional<std::string> baseURL;


dhilst · 2025-01-30T13:51:27Z

include/cloysterhpc/services/repo.h

+    void unparse( const std::vector<ELRepo>& repositories );
+    void unparse( const std::vector<ELRepo>& repositories, std::stringstream& ss );


Suggested change

void unparse( const std::vector<ELRepo>& repositories );

void unparse( const std::vector<ELRepo>& repositories, std::stringstream& ss );

void unparse(const std::vector<ELRepo>& repositories);

void unparse(const std::vector<ELRepo>& repositories, std::stringstream& ss);

dhilst · 2025-01-30T14:04:20Z

src/services/repo.cpp

+    auto get_optional_string
+        = [this](auto group, auto key) -> std::optional<Glib::ustring> {
+        return m_file->has_key(group, key)
+            ? std::make_optional(m_file->get_string(group, key))
+            : std::nullopt;
+    };


I don't see why this captures this, anyway, move it to functions.cpp

dhilst

The API looks good, there are a few points

Make sure to remove the prints
Rename the ELReo to ELCloneRepo
Remove the PR from draft

viniciusferrao

Another issue that I've observed. The virtual classes should be on its own files. This needs to be fixed.

viniciusferrao · 2025-01-30T13:54:28Z

src/services/repo.cpp

+
+std::vector<ELRepo> ELRepoFile::parse()
+{
+    this->read();


Do we need "this"?

viniciusferrao · 2025-01-30T13:56:12Z

src/services/repo.cpp


        if (name.empty()) {
            throw repository_exception(std::format(
-                "Could not load repo name from repo '{}' at file {}",
+                "Could not load repo name from repo '{}' at m_file {}",


This is more than a DEBUG message for the developer than to the user. For the user it should be something more friendly.

What do you suggest?

viniciusferrao · 2025-01-31T14:55:44Z

include/cloysterhpc/functions.h

@@ -164,6 +166,9 @@ std::string findAndReplace(const std::string_view& source,
 */
 void copyFile(std::filesystem::path source, std::filesystem::path destination);

+std::optional<Glib::ustring> readKeyfileString(Glib::RefPtr<Glib::KeyFile> file,
+    const std::string_view& group, const std::string_view& key);


std:string_view is already a pointer to a string. It should not be passed as const ref.

I didn't know that. I thought it was pointer+length. It will be fixed

viniciusferrao · 2025-01-31T14:56:36Z

include/cloysterhpc/services/repo.h

 /**
 * Repository file class
 *
 * This class should parse the repository data
 */
 class ELRepoFile : GenericFile {
-private:    
+private:
    Glib::RefPtr<Glib::KeyFile> m_file;

    std::vector<ELRepo> parseData();
    void unparseData(const std::vector<ELRepo>&);


Missing the variable name.

The variable name is not needed in the declaration and the context is obvious, but I will fixed because of conventions

viniciusferrao · 2025-02-01T02:03:58Z

src/functions.cpp

@@ -351,4 +351,12 @@ void copyFile(std::filesystem::path source, std::filesystem::path destination)
    }
 }

+std::optional<Glib::ustring> readKeyfileString(Glib::RefPtr<Glib::KeyFile> file,
+    const std::string_view& group, const std::string_view& key)


No need to be const ref.

See this comment

viniciusferrao

I ended up adding a lot of comments.

I think we should think a little bit more on this implementation.

viniciusferrao · 2025-02-07T01:09:08Z

include/cloysterhpc/file.h

The class name must match de filename. And also the #ifndef guards.

I would recommend using a namespace to avoid clashing.

viniciusferrao · 2025-02-07T01:15:30Z

include/cloysterhpc/file.h

+    virtual void read() { }
+    virtual void write() { }


This should be = 0? I can't find the implementation in other files, only the specialization. It should be pure virtual.

viniciusferrao · 2025-02-07T01:19:22Z

include/cloysterhpc/file.h

+    {
+    }
+
+    virtual void read() { }


I think read will not modify the state of the object right? Should it be const?

viniciusferrao · 2025-02-07T01:20:45Z

include/cloysterhpc/file.h

+    std::filesystem::path m_path;
+
+public:
+    explicit GenericFile(const std::filesystem::path& path)


Are we missing overloads? std::string_view, std::string and const char *? Should it be templated with Concepts?

I would not add the overloads if it is not being used, I would make the m_path const though

viniciusferrao · 2025-02-07T01:26:31Z

include/cloysterhpc/file.h

+    virtual void read() { }
+    virtual void write() { }
+
+    virtual ~GenericFile() = default;


Since you defaulted a destructor, AFAIK, the move operations are by default deleted. I think we should define the move operations:

GenericFile(GenericFile&&) = default; GenericFile& operator=(GenericFile&&) = default;

viniciusferrao · 2025-02-07T02:03:02Z

src/services/repofile.cpp

+void ELRepoFile::parse()
+{
+    read();
+
+    m_repositories = parseData();
+}


read() and parse() are both public. However parse() calls read() obfuscating it.

Is that the behaviour you intended to?

I have some thoughts about this class, I will review its API after looking into the repo manager. Read is public because it inherits public from GenericFile I guess.

viniciusferrao · 2025-02-07T02:06:11Z

src/services/repofile.cpp

+        repo.enabled = enabled;
+        repo.gpgcheck = gpgcheck;
+        repo.gpgkey = gpgkey;
+        repositories.push_back(std::move(repo));


emplace_back.

viniciusferrao · 2025-02-07T02:07:39Z

src/services/repofile.cpp

+        ELCloneRepo repo;
+        repo.group = repogroup.raw();
+        repo.name = name.raw();
+        repo.metalink
+            = metalink.transform([](const auto& v) { return v.raw(); });
+        repo.base_url
+            = baseurl.transform([](const auto& v) { return v.raw(); });
+        repo.enabled = enabled;
+        repo.gpgcheck = gpgcheck;
+        repo.gpgkey = gpgkey;


Wouldn't be better to use an initializer list?

This approach may lead to uninitialized values.

viniciusferrao · 2025-02-07T02:11:25Z

src/services/repofile.cpp

+}
+
+[[nodiscard]] const std::vector<ELCloneRepo>&
+ELRepoFile::getRepositoriesConst() const


Having the qualifier in the method name is a very bad approach.

viniciusferrao · 2025-02-07T02:16:42Z

src/services/repofile.cpp

+        m_file->load_from_file(m_path.string());
+    } catch (Glib::FileError& e) {
+        throw repository_exception(
+            std::format("Could not load repository file {} ({})",


fmt::format.

dhilst · 2025-02-12T19:44:55Z

include/cloysterhpc/presenter/PresenterNodesOperationalSystem.h

@@ -84,4 +86,6 @@ class PresenterNodesOperationalSystem : public Presenter {
        std::unique_ptr<Cluster>& model, std::unique_ptr<Newt>& view);
 };

+};


Add namespace closing comment

dhilst · 2025-02-12T19:56:07Z

include/cloysterhpc/models/elclonerepo.h

@@ -0,0 +1,18 @@
+//


Remove this file

dhilst · 2025-02-12T19:57:46Z

include/cloysterhpc/functions.h

@@ -164,6 +172,9 @@ std::string findAndReplace(const std::string_view& source,
 */
 void copyFile(std::filesystem::path source, std::filesystem::path destination);

+std::optional<Glib::ustring> readKeyfileString(Glib::RefPtr<Glib::KeyFile> file,


Remove this

dhilst · 2025-02-12T20:06:06Z

include/cloysterhpc/models/cluster.h

 #include <cloysterhpc/ofed.h>
-#include <cloysterhpc/queuesystem/pbs.h>
-#include <cloysterhpc/queuesystem/slurm.h>
-#include <cloysterhpc/repos.h>
 #include <cloysterhpc/runner.h>


Move runner to services

dhilst · 2025-02-12T20:07:25Z

include/cloysterhpc/models/pbs.h

@@ -23,4 +26,6 @@ class PBS : public QueueSystem {
    explicit PBS(const Cluster& cluster);
 };

+};


Suggested change

};

}; // namespace cloyster::models

dhilst · 2025-02-12T20:10:03Z

include/cloysterhpc/presenter/PresenterQueueSystem.h

@@ -42,4 +43,6 @@ class PresenterQueueSystem : public Presenter {
        std::unique_ptr<Cluster>& model, std::unique_ptr<Newt>& view);
 };

+};


Add namespace closing comment

dhilst · 2025-02-12T20:10:12Z

include/cloysterhpc/presenter/PresenterRepository.h

@@ -24,3 +25,5 @@ class PresenterRepository : public Presenter {
    PresenterRepository(
        std::unique_ptr<Cluster>& model, std::unique_ptr<Newt>& view);
 };
+
+};


Add namespace closing comment

dhilst · 2025-02-12T20:10:18Z

include/cloysterhpc/presenter/PresenterTime.h

@@ -38,4 +40,6 @@ class PresenterTime : public Presenter {
    PresenterTime(std::unique_ptr<Cluster>& model, std::unique_ptr<Newt>& view);
 };

+};


Add namespace closing comment

dhilst · 2025-02-12T20:10:22Z

include/cloysterhpc/presenter/PresenterWelcome.h

@@ -29,5 +30,6 @@ class PresenterWelcome : public Presenter {
    PresenterWelcome(
        std::unique_ptr<Cluster>& model, std::unique_ptr<Newt>& view);
 };
+};


Add namespace closing comment

dhilst · 2025-02-12T20:10:46Z

include/cloysterhpc/services/files.h

+template <typename File>
+concept IsKeyFileWriteable = requires(File& file, const std::string& group, const std::string& key, bool bvalue, const std::string& svalue, const std::optional<std::string>& soptvalue)
+{
+        file.setString(group, key, svalue);
+        file.setString(group, key, soptvalue);
+        file.setBoolean(group, key, bvalue);
+};


dhilst · 2025-02-12T20:51:42Z

src/models/answerfile.cpp

@@ -605,6 +607,8 @@ void AnswerFile::loadPostfix()
    postfix.key_file = m_ini.getValue("postfix", "smtpd_tls_key_file", false);
 }

+};


Add namespace closing comment

dhilst · 2025-02-12T20:52:10Z

src/models/cluster.cpp

+// void Cluster::initRepoManager()
+// {
+//     if (cloyster::dryRun) {
+//        m_repos.emplace(RepoManager<repository, DryRunner>(*getRunner(), m_headnode.getOS()));
+//     } else {
+//        m_repos.emplace(RepoManager<repository, Runner>(*getRunner(), m_headnode.getOS()));
+//     }
+// }
+
+// @TODO Remove this after as part of the repository refactoring
+// template <typename Runner>
+// RepoManager<repository, Runner>& Cluster<Runner>::getRepoManager()
+// {
+//     if (!m_repos) {
+//         initRepoManager();
+//     }
+//
+//     return m_repos.value();
+// }
+
+// RepoManager<repository, BaseRunner>& Cluster::getRepoManager()
+// {
+//      if (!m_repos) {
+//          m_repos = cloyster::getRepoManager();
+//      };
+//      return m_repos.value();
+//  }


Remove commented code

dhilst · 2025-02-12T20:52:32Z

src/models/cpu.cpp

@@ -66,3 +70,5 @@ void CPU::setThreadsPerCore(size_t threadsPerCore)
 {
    m_threadsPerCore = threadsPerCore;
 }
+
+}


Add closing namespace comment

dhilst · 2025-02-12T20:52:39Z

src/models/headnode.cpp

@@ -54,3 +57,5 @@ void Headnode::setBootTarget(Headnode::BootTarget bootTarget)
 {
    m_bootTarget = bootTarget;
 }
+
+}; 


Add namespace closing comment

dhilst · 2025-02-12T20:52:43Z

src/models/node.cpp

@@ -46,3 +48,5 @@ void Node::setNodeRootPassword(
 {
    m_node_root_password = nodeRootPassword;
 }
+
+}


Add namespace closing comment

dhilst · 2025-02-12T20:52:48Z

src/models/os.cpp

@@ -267,4 +269,6 @@ void OS::printData() const
    LOG_DEBUG("Major Version: {}", m_majorVersion)
    LOG_DEBUG("Minor Version: {}", m_minorVersion)
 }
+
+};


Add namespace closing comment

dhilst · 2025-02-12T20:52:52Z

src/models/pbs.cpp

@@ -17,3 +19,5 @@ void PBS::setExecutionPlace(ExecutionPlace executionPlace)
 }

 PBS::ExecutionPlace PBS::getExecutionPlace(void) { return m_executionPlace; }
+
+}


Add namespace closing comment

dhilst · 2025-02-12T20:52:59Z

src/models/queuesystem.cpp

@@ -20,3 +22,5 @@ void QueueSystem::setDefaultQueue(std::string_view defaultQueue)
 }

 std::string_view QueueSystem::getDefaultQueue(void) { return m_defaultQueue; }
+
+};


Add namespace closing comment

dhilst · 2025-02-12T20:53:12Z

src/models/server.cpp

@@ -1,12 +1,15 @@
 /*
+ * o


Suggested change

* o

dhilst · 2025-02-12T20:53:18Z

src/models/server.cpp

@@ -173,3 +176,5 @@ TEST_SUITE("Test FQDN")
        }
    }
 }
+
+}


Add namespace closing comment

dhilst · 2025-02-12T20:53:31Z

src/models/slurm.cpp

@@ -59,3 +60,5 @@ void SLURM::startServer()
    runCommand("systemctl start munge");
    runCommand("systemctl start slurmctld");
 }
+
+}


Add namespace closing comment

dhilst · 2025-02-12T20:53:41Z

src/presenter/CMakeLists.txt

Remove this file

dhilst · 2025-02-12T20:54:44Z

src/presenter/PresenterRepository.cpp

+    const std::vector<services::repos::BaseRepository>& repo_names = manager->listRepos();

-    auto repo_names = manager.listRepos();
    std::vector<std::tuple<std::string, std::string, bool>> repo_strings;

    std::transform(repo_names.begin(), repo_names.end(),
-        std::back_inserter(repo_strings), [](auto entry) {
-            return std::make_tuple(std::string { entry.id },
-                std::string { entry.name }, entry.enabled);
+        std::back_inserter(repo_strings), [](auto& entry) {
+            return std::make_tuple(std::string { entry.id() },
+                std::string { entry.name() }, entry.enabled() );
        });
+    assert(repo_strings.size() > 0); // reminder to fix this


I'm favoring to move stuff to models/ or services/ folders/namespaces. There is a lot of moves to do yet but this already outlines some structure

dhilst · 2025-02-12T21:33:15Z

include/cloysterhpc/concepts.h

+// @TODO: Move this to its own file
+/**
+ * @brief ParserNoExc<P, T, E> means: P can parse and unparse Ts from streams.
+ * The parsers are not allowed to throw exceptions, it must return errors of
+ * type E
+ */
+template <typename Parser_, typename T, typename E>
+concept ParserNoExc = requires(Parser_ parser, std::istream& input,
+    std::ostream& output, const T& parsed) {
+    { parser.parse(input) } noexcept -> std::same_as<std::expected<T, E>>;
+    {
+        parser.unparse(parsed, output)
+    } noexcept -> std::same_as<std::expected<void, E>>;
+};


This can go away

dhilst · 2025-02-12T21:33:26Z

include/cloysterhpc/concepts.h

+
+// @TODO: Move this to its own file
+/**


Remove this comment

dhilst · 2025-02-12T21:33:37Z

include/cloysterhpc/concepts.h

+    { parser.unparse(unparseInput, unparseOutput) } -> std::same_as<void>;
+};
+
+// @TODO: Move this to its own file


Suggested change

// @TODO: Move this to its own file

sonarqubecloud · 2025-02-12T22:40:33Z

Quality Gate failed

Failed conditions
21 Security Hotspots

See analysis details on SonarQube Cloud

WIP: Repository class refactoring

a01df8c

arthurmco requested a review from dhilst January 27, 2025 14:11

arthurmco self-assigned this Jan 27, 2025

viniciusferrao requested changes Jan 27, 2025

View reviewed changes

dhilst reviewed Jan 27, 2025

View reviewed changes

include/cloysterhpc/services/repo.h Outdated Show resolved Hide resolved

dhilst reviewed Jan 27, 2025

View reviewed changes

arthurmco added 2 commits January 30, 2025 08:31

Add parse() and unparse() methods, fix more things

6fe6003

Add stringstream read specialization, as suggested by @dhilst

99f79ce

dhilst reviewed Jan 30, 2025

View reviewed changes

arthurmco added 3 commits January 31, 2025 09:38

Move some common functions to functions.h

3f0f219

Remove this->

37f8b60

Fix repo class

ab4ba11

dhilst approved these changes Jan 31, 2025

View reviewed changes

dhilst requested changes Jan 31, 2025

View reviewed changes

viniciusferrao requested changes Feb 1, 2025

View reviewed changes

arthurmco added 2 commits February 3, 2025 10:30

Fix const ref and declaration name

25df8ed

Fix naming

de966e5

arthurmco requested a review from dhilst February 4, 2025 13:44

arthurmco marked this pull request as ready for review February 4, 2025 13:45

arthurmco added 2 commits February 4, 2025 12:37

Rename required files

4895e2e

fixup! Rename required files

a834f94

viniciusferrao requested changes Feb 7, 2025

View reviewed changes

dhilst assigned dhilst and unassigned arthurmco Feb 7, 2025