Fix repository listing and selection #71

arthurmco · 2024-08-30T14:50:18Z

Allow the user to see and enable/disable the repositories that will be installed in the machine.

Also, change the way we handle repositories, by:

configuring them in memory and save every change in one go
use the .repos embedded in the source only as a template, instead of saving it directly

This way, we can easily discover all repos in the machine

from outside, from an interface or answerfile Also add some tests to see if we parsed the data correctly

lbgracioso

LGTM.

viniciusferrao · 2024-09-02T14:30:29Z

src/repos.cpp

-    auto path = std::string { tmpnam(nullptr) };
-    std::filesystem::create_directory(m_path);
-    m_path = m_path;
+    char* temp = "tempdirXXXXXX";   


This is not good. If we gonna create this it would be better to be constexpr.

viniciusferrao · 2024-09-02T14:31:05Z

src/repos.cpp

-    m_path = m_path;
+    char* temp = "tempdirXXXXXX";   
+
+    auto path = std::string { mkdtemp(temp) };


You cannot create it directly? mkdtemp("tempdirXXXXXX")? And is this correct? tempdirXXXXXX? It's ugly.

viniciusferrao · 2024-09-04T23:08:15Z

SonarCloud is still upset @arthurmco.

Is there a way to just disable the specific error? I see that you added //NOSONAR, however this will disable SonarCloud completely in the line.

There's no way to select a block of code and only disable the specific bogus error?

Finally the code duplication issue, I didn't get it. It's also a bogus issue?

EDIT: https://gist.github.com/brian-ahr/a5bf071feda62ffbc5e1686c0cafb52c

arthurmco · 2024-09-05T14:59:56Z

Is there a way to just disable the specific error? I see that you added //NOSONAR, however this will disable SonarCloud completely in the line.

There are some ways, like specifying #pragmas and annotations, but they only seem to work in certain languages (namely C# and Java)

There's no way to select a block of code and only disable the specific bogus error?

For C++, currently, no.

Finally the code duplication issue, I didn't get it. It's also a bogus issue?

Probably yes, because I did not duplicate nothing. Sonarcloud did not tell exactly what have been duplicated

arthurmco · 2024-09-06T14:31:33Z

Finally

src/repos.cpp

It complains about a pseudo-RNG in a function that is rarely used, and used only for testing, almost never in production

@viniciusferrao

> @viniciusferrao: There's a class that defines version: > ```cpp > enum class Platform { el8, el9 }; > ```

viniciusferrao · 2024-09-11T16:15:02Z

src/view/newtListMenu.cpp

+        if (es.u.co == bOk) {
+            retval = 1;
+        } else if (es.u.co == bHelp) {
+            this->helpMessage(help);
+        } else {
+            retval = 2;
+        }


switch?

Also is this the handling of the buttons? If yes we have a standard way to handle it:

cloysterhpc/src/view/newtYesNoQuestion.cpp

Line 29 in ed75e5d

goto question;

However I think that should be another method, to avoid code duplication. Not sure how to achieve that on the context.

switch?

The, switch only would work here if the bOk and bHelp were constexpr; and I don't think this is applicable, because the button creation function is not constexpr

Also is this the handling of the buttons? If yes we have a standard way to handle it:

Yes, but since this is a custom-built form (because we needed to select multiple repos, something that the default list form does not allow us to do), we do not have the error code treatment that the easy form functions Newt give to us

However I think that should be another method, to avoid code duplication. Not sure how to achieve that on the context.

I agree, but I need to think a little bit more about that

Ok @arthurmco if you didn't came up with a solution in a timely manner I think we can accept this. I was wondering for a total overhaul of the TUI in the future.

However here, at least if you don't mind using a better naming in the variables, like button instead of b. Is good. To be hones is a little bad to read es.u.co == b0k.

Ok. Did it in the last commit

viniciusferrao · 2024-09-11T16:20:47Z

src/view/newtListMenu.cpp

+    newtFormSetWidth(form, m_suggestedWidth);
+
+    auto* label = newtLabel(1, 1, message);
+    auto stitle = std::string { title };


Don't get it. Is this an implicit type conversion?

Yes

The compiler can deduce the type anyway; and since this label does not leave the function, the exact type name isn't too important

Alright. Just use title directly no? In the end is another string. However if the plan is to store a const char* well... just use title.c_str() directly.

src/view/newtListMenu.cpp

viniciusferrao · 2024-09-11T16:23:26Z

src/view/newtListMenu.cpp

+        NEWT_GRID_FLAG_GROWX | NEWT_GRID_FLAG_GROWY);
+    newtGridSetField(grid, 0, 2, NEWT_GRID_SUBGRID, buttonGrid, 0, 1, 0, 0, 0,
+        NEWT_GRID_FLAG_GROWX);
+    newtGridWrappedWindow(grid, const_cast<char*>(stitle.c_str()));


Hmmm do we need creating a local copy?

Of what? Of the string?

If you are talking about stitle, then yes. This function (newtGridWrappedWindow) is not const-correct, and I created a "sacrificial" copy to preserve the original string, in the rare case this function does change the string

Of what? Of the string?

If you are talking about stitle, then yes. This function (newtGridWrappedWindow) is not const-correct, and I created a "sacrificial" copy to preserve the original string, in the rare case this function does change the string

What if we const_cast?

This will violate constness of the original value

Take a look on the comment on the auto implicit conversion.

src/services/shell.cpp

viniciusferrao · 2024-09-11T16:29:54Z

include/cloysterhpc/cloyster.h

+extern bool showVersion;
+extern bool runAsRoot;
+extern bool dryRun;
+extern bool enableTUI;
+extern bool enableCLI;
+extern bool runAsDaemon;
+
+extern std::string logLevelInput;
+extern std::string answerfile;
+extern std::string customRepofilePath;


I don't know a better solution to this. This is not a change request and only a comment, but I don't like this either. It's not your code, the old code wasn't good at all also.

The old code duplicated the variable instantiations (for some reason, even though we had the include guards)

I moved them to an external file, leaving only the declarations.

I also don't like this, but I could not think about any other way

The other solution would be using a class where every variable is public and static (or a struct where every member is static), but this would take some time to refactor

viniciusferrao · 2024-09-11T16:31:04Z

include/cloysterhpc/repos/el9/cloyster.repo

I think we should abandon the [cloyster-{}] name on the repos. That should reflect PRODUCT_NAME or just use the default names. I'm open to suggestions here.

viniciusferrao · 2024-09-11T16:31:23Z

include/cloysterhpc/runner.h

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#pragma once


This one should use #ifndef instead.

src/presenter/PresenterRepository.cpp

src/view/newtListMenu.cpp

viniciusferrao

There's still some considerations here @arthurmco; please take a look if you can.

viniciusferrao · 2024-09-20T05:13:20Z

include/cloysterhpc/runner.h

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#pragma once


This one should use #ifndef instead.

viniciusferrao · 2024-09-20T05:18:20Z

src/runner.cpp

+int DryRunner::executeCommand(const std::string& cmd)
+{
+    LOG_INFO("Would execute command: {}", cmd);
+    return 0;


Suggested change

return 0;

return OK;

Does this return is even necessary? It always return 0.

Yes (see below)

viniciusferrao · 2024-09-20T05:18:56Z

src/runner.cpp

+int MockRunner::executeCommand(const std::string& cmd)
+{
+    m_cmds.push_back(cmd);
+    return 0;


Since this is the only return does it makes sense? It always return 0.

Yes

This class descends from the BaseRunner. One of its child is the normal Runner, which really runs a command and returns a value depending on said command return code

viniciusferrao · 2024-09-20T05:19:46Z

src/services/shell.cpp

+    auto repos = m_cluster->getRepoManager();
+    repos.loadFiles();
+    repos.enableMultiple({ "cloyster-beegfs", "cloyster-elrepo",
+        "cloyster-epel", "cloyster-openhpc",


What about the names here? We have mentioned to use PRODUCT_NAME instead or just use the default distribution names.

viniciusferrao · 2024-09-20T05:20:33Z

src/tempdir.cpp

+#include <fmt/format.h>
+#include <random>
+
+static char value_to_char(unsigned long value)


Suggested change

static char value_to_char(unsigned long value)

static char value_to_char(std::uint64_t value)

This would make more sense because it express intent.

viniciusferrao · 2024-09-20T05:22:18Z

test/sample/inifile.ini

Suggested change

company_name=cloyster-enterprises

company_name=example-enterprise

viniciusferrao · 2024-09-20T05:22:32Z

test/sample/inifile.ini

+[another]
+another_key=123
+second_key=ABC


Suggested change

second_key=ABC

second_key=ABC

Signed-off-by: Vinícius Ferrão <[email protected]>

sonarqubecloud · 2024-09-20T23:15:07Z

Quality Gate passed

Issues
31 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

viniciusferrao

Finally this one make through.

arthurmco added 3 commits August 28, 2024 11:36

inifile: add functions to list sections

b0c79c1

This way, we can easily discover all repos in the machine

repos: refactor it so we can enable/disable repos...

c349396

from outside, from an interface or answerfile Also add some tests to see if we parsed the data correctly

repos: add view to select repos

0f51102

arthurmco self-assigned this Aug 30, 2024

lbgracioso approved these changes Sep 2, 2024

View reviewed changes

repos: fix security issue

d9bccba

viniciusferrao reviewed Sep 2, 2024

View reviewed changes

repos: allow the user to set repos on interactive install

619a45f

lbgracioso self-requested a review September 3, 2024 08:42

viniciusferrao requested changes Sep 9, 2024

View reviewed changes

src/repos.cpp Outdated Show resolved Hide resolved

arthurmco added 4 commits September 11, 2024 07:43

repos: fix temporary directory name generation

b2b3fa3

Disable bogus sonarcloud notification

40977a5

It complains about a pseudo-RNG in a function that is rarely used, and used only for testing, almost never in production

Solve Sonarcloud duplicated code warning

36ddb48

repos: use the correct version enum

b3629ae

> @viniciusferrao: There's a class that defines version: > ```cpp > enum class Platform { el8, el9 }; > ```

arthurmco force-pushed the feat/fix-repos branch from ed52cad to b3629ae Compare September 11, 2024 10:44

viniciusferrao requested changes Sep 11, 2024

View reviewed changes

Implement corrections suggested by @viniciusferrao

4f6d8b7

arthurmco force-pushed the feat/fix-repos branch from 5c3ee50 to 4f6d8b7 Compare September 16, 2024 14:42

arthurmco requested a review from viniciusferrao September 17, 2024 14:35

viniciusferrao requested changes Sep 20, 2024

View reviewed changes

Correct issues on RepoManager class

bec3ec0

arthurmco requested a review from viniciusferrao September 20, 2024 23:00

Fix broken lines.

1a1c071

Signed-off-by: Vinícius Ferrão <[email protected]>

viniciusferrao approved these changes Sep 20, 2024

View reviewed changes

viniciusferrao merged commit 9867da5 into master Sep 20, 2024
3 checks passed

viniciusferrao deleted the feat/fix-repos branch January 18, 2025 02:19

	static char value_to_char(unsigned long value)
	static char value_to_char(std::uint64_t value)

	company_name=cloyster-enterprises
	company_name=example-enterprise

Fix repository listing and selection #71

Fix repository listing and selection #71

Conversation

arthurmco commented Aug 30, 2024

lbgracioso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viniciusferrao commented Sep 4, 2024 • edited Loading

arthurmco commented Sep 5, 2024

arthurmco commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viniciusferrao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viniciusferrao Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 20, 2024

Quality Gate passed

viniciusferrao left a comment

Choose a reason for hiding this comment

viniciusferrao commented Sep 4, 2024 •

edited

Loading

viniciusferrao Sep 20, 2024 •

edited

Loading