Shell selection support via shebang #249

New Issue

ungleich-gitea · 2021-11-20T15:22:29Z

ungleich-gitea commented

2021-11-20 15:22:29 +00:00

Created by: dheule

On the local host, it would be cool to allow the user
to select which shell cdist uses to execute the manifests and gencode-* instead of /bin/sh.

Background: I don't want to replace /bin/sh with dash, but want test if my scripts are working faster with dash.

I think it would be the best way to make cdist look for the shebang line in the manifest/init file and use the selected shell for execution.

So the admin can leave the system untouched and only cdist uses a special shell.

examples:

#!/bin/sh  --> execute with /bin/sh
#!/bin/dash --> execute the manifest and gencode with dash

*Created by: dheule* On the local host, it would be cool to allow the user to select which shell cdist uses to execute the manifests and gencode-\* instead of /bin/sh. Background: I don't want to replace /bin/sh with dash, but want test if my scripts are working faster with dash. I think it would be the best way to make cdist look for the shebang line in the manifest/init file and use the selected shell for execution. So the admin can leave the system untouched and only cdist uses a special shell. examples: ``` #!/bin/sh --> execute with /bin/sh #!/bin/dash --> execute the manifest and gencode with dash ```

ungleich-gitea added this to the future milestone 2021-11-20 15:22:29 +00:00

ungleich-gitea closed this issue

2021-11-20 15:22:29 +00:00

ungleich-gitea commented

2021-11-20 15:50:04 +00:00

Created by: dheule

This bug is fixed not via shebang, but via ENV variables:
CDIST_LOCAL_SHELL for local scripts
CDIST_REMOTE_SHELL for remote scripts

So i close this issue.

*Created by: dheule* This bug is fixed not via shebang, but via ENV variables: CDIST_LOCAL_SHELL for local scripts CDIST_REMOTE_SHELL for remote scripts So i close this issue.

ungleich-gitea commented

2021-11-20 15:50:05 +00:00

Created by: dheule

Hello Nico,

2013/12/18 Nico Schottelius notifications@github.com

Shebang would definitely be possible (we could probably use a python
function to figure this out), but I am not sure if this is a good idea, as
most manifest/gencode-* scripts are coming without the header.

My idea was not to examine every script,
only to look at the first file in the manifest e.g. manifest/init
if there is a shebang line, execute all scripts on the central host with
this shell,
if not, go to default value /bin/sh like now.

Another solution is to have a config file on wich the now hard coded shell
can be selected.

But your idea has also some advantages,
if correctly implemented gencode and all manifests
could be written in every aviable script language ...

Let's put it that way: I think it's a good idea to support it, but I would
be cautious to not break something for the moment (although the 3.x.x
release could break things, which is upcoming this year...)

—
Reply to this email directly or view it on GitHubhttps://github.com/telmich/cdist/issues/232#issuecomment-30858546
.

*Created by: dheule* Hello Nico, 2013/12/18 Nico Schottelius notifications@github.com > Shebang would definitely be possible (we could probably use a python > function to figure this out), but I am not sure if this is a good idea, as > most manifest/gencode-\* scripts are coming without the header. > > My idea was not to examine every script, > only to look at the first file in the manifest e.g. manifest/init > if there is a shebang line, execute all scripts on the central host with > this shell, > if not, go to default value /bin/sh like now. Another solution is to have a config file on wich the now hard coded shell can be selected. But your idea has also some advantages, if correctly implemented gencode and all manifests could be written in every aviable script language ... > Let's put it that way: I think it's a good idea to support it, but I would > be cautious to not break something for the moment (although the 3.x.x > release could break things, which is upcoming this year...) > > — > Reply to this email directly or view it on GitHubhttps://github.com/telmich/cdist/issues/232#issuecomment-30858546 > .

ungleich-gitea commented

2021-11-20 15:50:06 +00:00

Created by: telmich

Shebang would definitely be possible (we could probably use a python function to figure this out), but I am not sure if this is a good idea, as most manifest/gencode-* scripts are coming without the header.

Let's put it that way: I think it's a good idea to support it, but I would be cautious to not break something for the moment (although the 3.x.x release could break things, which is upcoming this year...)

*Created by: telmich* Shebang would definitely be possible (we could probably use a python function to figure this out), but I am not sure if this is a good idea, as most manifest/gencode-\* scripts are coming without the header. Let's put it that way: I think it's a good idea to support it, but I would be cautious to not break something for the moment (although the 3.x.x release could break things, which is upcoming this year...)

tabulon commented

2023-03-28 02:24:43 +00:00

Hi,

Well, I am not a fan of the solution involving the CDIST_LOCAL_SHELL and CDIST_REMOTE_SHELL environment variables, because of the all-or-nothing nature.

The shebang mecanism is directly supported by Unix/Linux kernels. Therefore, in order to support it, cdist doesn't really need to parse or do anything clever with it. On the contrary, cdist would need to abstain from doing too much... Just sticking with KISS will do.

BTW, the current cdist version (v7.0.0) appears to actually support the shebang line whenever it runs a script locally, as long the script file has EXECUTE permissions set, without a need to set the CDIST_LOCAL_SHELL environment variable.

This appears to work fine for manifests and gencode-* scripts. You just need to make sure the shebang (#!...) line is there and the EXECUTE bit is set on the script file.

Note that, currently, you can't practically achieve this for the generated code-local, because cdist (on the python side) doesn't seem set the EXECUTE bit on the generated code-* scripts.

Below is an excerpt from local.py. When the file has EXECUTE permissions (i.e. if os.access(script, os.X_OK):, the given script is directly executed (instead of being pass as an argument to /bin/sh).

The simple and direct for of execution (of the given script) is actually what enables the native shebang* support of the Kernel itself.

When the script is fed into /bin/sh (without the -c option), the shell, by design, won't honor the shebang line. It's basically forced to interpret the script itself in that case...


    def run_script(self, script, env=None, return_output=False,
                   message_prefix=None, stdout=None, stderr=None):
        """Run the given script with the given environment.
        Return the output as a string.
        """
        if os.access(script, os.X_OK):
            self.log.debug('%s is executable, running it', script)
            command = [os.path.realpath(script)]
        else:
            command = [self.configuration.get('local_shell', "/bin/sh"), "-e"]
            self.log.debug('%s is NOT executable, running it with %s',
                           script, " ".join(command))
            command.append(script)
        return self.run(command, env=env, return_output=return_output,
                        message_prefix=message_prefix, stdout=stdout,
                        stderr=stderr)

Unfortunately, this doesn't currently work for remote execution, which appears to systematically pass the given script to '/bin/sh' without any regard to the EXECUTE permissions, as below :

Here's how run_script is defined in remote.py :


    def run_script(self, script, env=None, return_output=False, stdout=None,
                   stderr=None):
        """Run the given script with the given environment on the remote side.
        Return the output as a string.
        """
        command = [
            self.configuration.get('remote_shell', "/bin/sh"),
            "-e"
        ]
        command.append(script)
        return self.run(command, env=env, return_output=return_output,
                        stdout=stdout, stderr=stderr

The solution is actually quite simple: Just mimic the way local.py handles the situation (except that the check for the EXECUTE bit needs to happen on the remote side as well).

I have cooked up a solution does just that, with quite minimal changes... And it appears to work fine on my end. If desired, I can submit a PR for the change.

To be on the safe side, I can try to guard the change with the BETA setting and perhaps yet another environment variable.

Note that this preliminary PR would NOT cover setting the EXECUTABLE bit on code-local and code-remote (which need to happen elsewhere, upon code generattion), but it would work wherever it is already set...

explorers work, be it global or type explorers.

I am jotting down the gist of the involved change in remote.py below, for eventual comments :


    def is_executable(self, script):
        """Check if the given remote script is executable (has its execute bit set).
        Return True/False

        """
        chk =("/bin/sh -c '" + 'f="' + script + '"; if [ -f "$f" ] && [ -x "$f" ] ; then echo OK ; else echo NOPE ; fi' + "'", "")
        self.log.trace('[@REMOTE] %-60s : checking file exec permissions via : %s ...', script, " ".join(chk))
        out = self.run(chk, env=None, return_output=True) or ""
        self.log.trace('[@REMOTE] %-60s : checked  file exec permissions out : %s', script, out)
        return out.strip() == 'OK'

    def run_script(self, script, env=None, return_output=False, stdout=None,
                   stderr=None):
        """Run the given script with the given environment on the remote side.
        Return the output as a string.

        """

        command = []
        if self.is_executable(script):
            self.log.debug('[@REMOTE] %-60s : file is executable, running it', script)
        else:
            command = [self.configuration.get('remote_shell', "/bin/sh"), "-e"]
            self.log.debug('[@REMOTE] %-60s : file is NOT executable, running it with %s',
                           script, " ".join(command))
        command.append(script)

        return self.run(command, env=env, return_output=return_output,
                        stdout=stdout, stderr=stderr)

Hi, Well, I am not a fan of the solution involving the `CDIST_LOCAL_SHELL` and `CDIST_REMOTE_SHELL` environment variables, because of the all-or-nothing nature. The **shebang** mecanism is directly supported by Unix/Linux kernels. Therefore, in order to support it, `cdist` doesn't really need to parse or do anything clever with it. On the contrary, `cdist` would need to abstain from doing too much... Just sticking with KISS will do. BTW, the current `cdist` version (v7.0.0) appears to actually support the **shebang** line whenever it runs a script **locally**, as long the script file has **EXECUTE** permissions set, without a need to set the CDIST_LOCAL_SHELL environment variable. This appears to work fine for `manifests` and `gencode-*` scripts. You just need to make sure the **shebang** (`#!...`) line is there and the **EXECUTE** bit is set on the script file. > Note that, currently, you can't practically achieve this for the generated `code-local`, because `cdist` (on the python side) doesn't seem set the **EXECUTE** bit on the generated `code-*` scripts. Below is an excerpt from `local.py`. When the file has EXECUTE permissions (i.e. `if os.access(script, os.X_OK):`, the given script is directly executed (instead of being pass as an argument to `/bin/sh`). The simple and direct for of execution (of the given script) is actually what enables the native **shebang*** support of the Kernel itself. When the script is fed into `/bin/sh` (without the `-c` option), the shell, by design, won't honor the shebang line. It's basically forced to interpret the script itself in that case... ```python def run_script(self, script, env=None, return_output=False, message_prefix=None, stdout=None, stderr=None): """Run the given script with the given environment. Return the output as a string. """ if os.access(script, os.X_OK): self.log.debug('%s is executable, running it', script) command = [os.path.realpath(script)] else: command = [self.configuration.get('local_shell', "/bin/sh"), "-e"] self.log.debug('%s is NOT executable, running it with %s', script, " ".join(command)) command.append(script) return self.run(command, env=env, return_output=return_output, message_prefix=message_prefix, stdout=stdout, stderr=stderr) ``` Unfortunately, this doesn't currently work for **remote** execution, which appears to systematically pass the given script to '/bin/sh' without any regard to the EXECUTE permissions, as below : Here's how `run_script` is defined in `remote.py` : ```python def run_script(self, script, env=None, return_output=False, stdout=None, stderr=None): """Run the given script with the given environment on the remote side. Return the output as a string. """ command = [ self.configuration.get('remote_shell', "/bin/sh"), "-e" ] command.append(script) return self.run(command, env=env, return_output=return_output, stdout=stdout, stderr=stderr ``` The solution is actually quite simple: Just mimic the way `local.py` handles the situation (except that the check for the EXECUTE bit needs to happen on the remote side as well). I have cooked up a solution does just that, with quite minimal changes... And it appears to work fine on my end. If desired, I can submit a PR for the change. To be on the safe side, I can try to guard the change with the BETA setting and perhaps yet another environment variable. Note that this preliminary PR would NOT cover setting the EXECUTABLE bit on `code-local` and `code-remote` (which need to happen elsewhere, upon code generattion), but it would work wherever it is already set... explorers work, be it global or type explorers. I am jotting down the gist of the involved change in `remote.py` below, for eventual comments : ```python def is_executable(self, script): """Check if the given remote script is executable (has its execute bit set). Return True/False """ chk =("/bin/sh -c '" + 'f="' + script + '"; if [ -f "$f" ] && [ -x "$f" ] ; then echo OK ; else echo NOPE ; fi' + "'", "") self.log.trace('[@REMOTE] %-60s : checking file exec permissions via : %s ...', script, " ".join(chk)) out = self.run(chk, env=None, return_output=True) or "" self.log.trace('[@REMOTE] %-60s : checked file exec permissions out : %s', script, out) return out.strip() == 'OK' def run_script(self, script, env=None, return_output=False, stdout=None, stderr=None): """Run the given script with the given environment on the remote side. Return the output as a string. """ command = [] if self.is_executable(script): self.log.debug('[@REMOTE] %-60s : file is executable, running it', script) else: command = [self.configuration.get('remote_shell', "/bin/sh"), "-e"] self.log.debug('[@REMOTE] %-60s : file is NOT executable, running it with %s', script, " ".join(command)) command.append(script) return self.run(command, env=env, return_output=return_output, stdout=stdout, stderr=stderr) ```

tabulon commented

2023-03-28 02:47:33 +00:00

For future reference (to self and others), here are two ugly workarounds that one can resort to within the script files themselves even in the absence of a proper fix on the cdist side.

The given examples are for perl, but the same conceptual mecanisms should be applicable also elsewhere... Your mileage may vary.

a) Just feed a HEREDOC to your favorite interpreter


#!/bin/sh

perl - <<-'__EOT__'

print "Hello World!\n"

__EOT__

b) Resort to UGLY ancient woodoo

The below is a very UGLY HACK unfortunately needed to get this script to be interpreted by perl (instead of 'sh') with the current version (7.00) of cdist, which does the following for remote invocation :

/bin/sh -c 'export __target_host=blabla; /bin/sh -e SCRIPT_FILE'

When the script is passed as a direct argument, /bin/sh effectively ignores the shebang line... BUMMER.

So we resort to a very ancient trick...

For a detailed explanation, SEE:

#!/usr/bin/env perl

# Do NOT remove the below two lines (and keep them as separate lines!)

eval 'exec /usr/bin/env perl -S $0 ${1+"$@"}'   # Both Perl and sh have an eval
    if 0;   # The 'if ..' must be kept on a separate line 
            # so that it is seen by ONLY perl which won't execute the 'eval'.

print "Hello World!\n";

For future reference (to self and others), here are two ugly workarounds that one can resort to within the script files themselves even in the absence of a proper fix on the `cdist` side. The given examples are for `perl`, but the same conceptual mecanisms should be applicable also elsewhere... Your mileage may vary. ### a) Just feed a HEREDOC to your favorite interpreter ```perl #!/bin/sh perl - <<-'__EOT__' print "Hello World!\n" __EOT__ ``` ### b) Resort to UGLY ancient woodoo The below is a very UGLY HACK unfortunately needed to get this script to be interpreted by perl (instead of 'sh') with the current version (7.00) of cdist, which does the following for remote invocation : ``` /bin/sh -c 'export __target_host=blabla; /bin/sh -e SCRIPT_FILE' ``` When the script is passed as a direct argument, /bin/sh effectively ignores the shebang line... BUMMER. So we resort to a very ancient trick... For a detailed explanation, SEE: - (https://unix.stackexchange.com/questions/450509/could-someone-explain-this-shebang-line-which-uses-sh-and-then-does-exec-perl) - [perlrun - how to execute the Perl interpreter - Perldoc Browser](https://perldoc.perl.org/perlrun) ```perl #!/usr/bin/env perl # Do NOT remove the below two lines (and keep them as separate lines!) eval 'exec /usr/bin/env perl -S $0 ${1+"$@"}' # Both Perl and sh have an eval if 0; # The 'if ..' must be kept on a separate line # so that it is seen by ONLY perl which won't execute the 'eval'. print "Hello World!\n"; ```

nico commented

2023-03-28 07:03:56 +00:00

Good morning @tabulon !

I think having identical behaviour on both sites makes a lot of sense.

I remember that the motivation for always using /bin/sh on the remote side was that in any case we generate the code locally and thus the assumption is that "it is always shell code".

Motivation behind that again is to only rely on the shell remotely.

Now you can argue, and it might be a good argument, to say that you can generate anything you want, including things that are interpreted by other shells.

And honestly, I don't think we should stop the user from doing that.

So +1 for making it a) consistent and b) allow the user to shoot themselves into the foot in case they generate something for which there is no interpreter. Like it, please submit a PR.

Good morning @tabulon ! I think having identical behaviour on both sites makes a lot of sense. I remember that the motivation for always using /bin/sh on the remote side was that in any case we generate the code locally and thus the assumption is that "it is always shell code". Motivation behind that again is to only rely on the shell remotely. Now you can argue, and it might be a good argument, to say that you can generate anything you want, including things that are interpreted by other shells. And honestly, I don't think we should stop the user from doing that. So +1 for making it a) consistent and b) allow the user to shoot themselves into the foot in case they generate something for which there is no interpreter. Like it, please submit a PR.

tabulon commented

2023-03-30 07:36:28 +00:00

Hi @nico,

Thank you for your prompt reaction and my apologies for the late reply.

It is certainly quite wise to keep assumptions about the remote host to a bare minimum.

And yes, you are absolutely right to point out that this sort of thing is a potential footgun, especially when the intended interpreter is not present on the target system.

On the bright side, even beyond the question of consistency, it opens up a whole new range of possibilities... when ubuiquity can be traded off for more power.

Besides :

some interpreters (like bash and perl) are pretty much guaranteed to be shipped with the base OS on many Unix/Linux flavors where the system plumming itself still relies on scripts written in those languages;
some tools such as awk and sed can be fed quite complex instructions that may be considered as a form of scripting;
for those interpreters that are less certain to be shipped with the base OS, it should be quite easy to declare a dependency in cdist (via require=), ensuring their presence before attempting to make any use of the corresponding language.

On the other hand, it is important that cdist itself (and the global explorers shipped with it) remain free of any additional assumptions beyond the presence of /bin/sh and a few rudimentary programs on the target system, though.

I understand you are already sold on the concept, for which I am very glad :-) Still jotted down the above as a future reference for at least part of the motivation/rationale for wanting this.

Yes, I will happily put together PR or two. It might take a while tough, since I am on quite a busy schedule these days.

Meanwhile, do you mind using this issue for a few questions that may rise ?

Hi @nico, Thank you for your prompt reaction and my apologies for the late reply. It is certainly quite wise to keep assumptions about the remote host to a bare minimum. And yes, you are absolutely right to point out that this sort of thing is a potential footgun, especially when the intended interpreter is not present on the target system. On the bright side, even beyond the question of consistency, it opens up a whole new range of possibilities... when ubuiquity can be traded off for more power. Besides : - some interpreters (like `bash` and `perl`) are pretty much guaranteed to be shipped with the base OS on many Unix/Linux flavors where the system plumming itself still relies on scripts written in those languages; - some tools such as `awk` and `sed` can be fed quite complex instructions that may be considered as a form of scripting; - for those interpreters that are less certain to be shipped with the base OS, it should be quite easy to declare a dependency in cdist (via require=), ensuring their presence before attempting to make any use of the corresponding language. On the other hand, it is important that `cdist` itself (and the global explorers shipped with it) remain free of any additional assumptions beyond the presence of `/bin/sh` and a few rudimentary programs on the target system, though. I understand you are already sold on the concept, for which I am very glad :-) Still jotted down the above as a future reference for at least part of the motivation/rationale for wanting this. Yes, I will happily put together PR or two. It might take a while tough, since I am on quite a busy schedule these days. Meanwhile, do you mind using this issue for a few questions that may rise ?

tabulon commented

2023-04-07 13:26:30 +00:00

Hi @nico,

At this point, I have got 3 branches, covering changes in different areas:

changes in exec/remote.py for honoring "execute" permissions on remote scripts

related tests

(solution as outlined above in this issue)

This, by itself, allows shebang support for explorers.

changes for polyglot support for generated code

(a relatively clean solution which introduces a new class FileScriptProperty(in fsproperty.py), that detects the presence of a shebang and sets "execute" bits on the persisted file)

changes to documentation

Would you prefer 1 or 2 PRs ?

I guess the best would be 2 PRs... since the the first change lays the ground and allows for shebang support on any executable remote script...

But the documentation changes already assume both changes... So it would be easier for me to package all of it in one PR... :-)

Also : does the project prefer the tests to be commited before code changes (as is usually the case) or the other way around (as is sometimes preferred) ?

Finally, at least for the time being, I have not protected any of this with a beta switch or any other configuration option, as I tend to think the risks are pretty low.

Let's see how we go about that after your review. OK?

Cheers,
Tabulo[n]

Hi @nico, At this point, I have got 3 branches, covering changes in different areas: 1) changes in `exec/remote.py` for honoring "execute" permissions on remote scripts + related tests _(solution as outlined above in this issue)_ This, by itself, allows **shebang** support for explorers. 2) changes for polyglot support for **generated code** _(a relatively clean solution which introduces a new class `FileScriptProperty`(in `fsproperty.py`), that detects the presence of a **shebang** and sets "execute" bits on the persisted file)_ 3) changes to **documentation** ____ Would you prefer 1 or 2 PRs ? I guess the best would be 2 PRs... since the the first change lays the ground and allows for shebang support on any executable remote script... But the documentation changes already assume both changes... So it would be easier for me to package all of it in one PR... :-) _________ **Also** : does the project prefer the tests to be commited before code changes (as is usually the case) or the other way around (as is sometimes preferred) ? _______ Finally, at least for the time being, I have not protected any of this with a **beta** switch or any other configuration option, as I tend to think the risks are pretty low. Let's see how we go about that after your review. OK? Cheers, Tabulo[n]

nico commented

2023-04-13 10:04:22 +00:00

Hell @tabulon !

Sorry for the late reply, a lot of things going on atm.

In regards to PRs: 1 would be preferred, including tests (where appropriate).

I just skimmed above comments and I think the direction is good, I'll need an evening tea and a bit of quiet time to fully review the changes so far.

Hell @tabulon ! Sorry for the late reply, a lot of things going on atm. In regards to PRs: 1 would be preferred, including tests (where appropriate). I just skimmed above comments and I think the direction is good, I'll need an evening tea and a bit of quiet time to fully review the changes so far.

tabulon referenced this issue

2023-04-15 03:36:29 +00:00

Make cdist truely language-agnostic (polyglot) #365

tabulon commented

2023-04-15 03:41:14 +00:00

Hi @nico,

Tell me about it... I am quite swamped on my end, too.

Just created the PR (#365) to make sure the ball is no longer on my court... :-)

No rush; whenever you can get to it.

Cheers,
Tabulon

Hi @nico, Tell me about it... I am quite swamped on my end, too. Just created the PR (#365) to make sure the ball is no longer on my court... :-) No rush; whenever you can get to it. Cheers, Tabulon

tabulon referenced this issue

2024-03-24 13:12:09 +00:00

Make cdist truely language-agnostic (polyglot) #365

tabulon commented

2024-03-24 13:13:17 +00:00

Hi @nico (or other maintainers of cdist),

Is there anything I can do expedite the review process around PR #365 which I had submitted about a year ago, following your initial encouragement on the fringes of this issue ?

FYI: Just in case the WIP prefix causes some notifications to be filtered out on your end, I have just removed that prefix and renamed the PR, which has anyhow been ready for review from the begining.

At this point in time, it will probably be necessary to rebase the PR on current master before it can be merged. I can take care of that once you complete the review -- or beforehand, if you really prefer.

Or else, if you see any issues with the feature or its implementation in the PR, please let me know.

(I am cross-posting this comment on #365 in order to increase its chances to get noticed).

Hi @nico (or other maintainers of cdist), Is there anything I can do expedite the review process around PR #365 which I had submitted about a year ago, following your initial encouragement on the fringes of this issue ? _FYI: Just in case the WIP prefix causes some notifications to be filtered out on your end, I have just removed that prefix and renamed the PR, which has anyhow been ready for review from the begining._ At this point in time, it will probably be necessary to rebase the PR on current master before it can be merged. I can take care of that once you complete the review -- or beforehand, if you really prefer. Or else, if you see any issues with the feature or its implementation in the PR, please let me know. (I am cross-posting this comment on #365 in order to increase its chances to get noticed).

nico commented

2024-03-25 07:50:14 +00:00

@tabulon Just wrapping my head around it, motivation looks great, now starting the review on #365.

tabulon commented

2024-03-25 09:13:55 +00:00

This is great news, @nico. Thank you.

Let me know if you have any comments / questions.

Note that the changes may appear vast at first : 22 files, many lines, ... as reported by git.

In fact, as you will notice, the actual changes to code are quite minimal. The bulk of it concerns the docs and tests.

Then again, for the tests, the change reported by git / diff appear to be much more than what it really is, mainly because I had to resort to burrying the original test class (CodeTestCase) in one of the test modules (/cdist/test/code/__init__.py), so as to be able to subclass it safely. This seems to have confused the diffalgorithm.

This is great news, @nico. Thank you. Let me know if you have any comments / questions. Note that the changes may appear vast at first : 22 files, many lines, ... as reported by `git`. In fact, as you will notice, the actual changes to code are quite minimal. The bulk of it concerns the docs and tests. Then again, for the tests, the change reported by `git / diff` appear to be much more than what it really is, mainly because I had to resort to burrying the original test class (`CodeTestCase`) in one of the test modules (`/cdist/test/code/__init__.py`), so as to be able to subclass it safely. This seems to have confused the `diff`algorithm.

fnux commented

2024-04-04 12:10:28 +00:00

@tabulon sorry it took so long for this to go forward. I can take over if @nico doesn't have enough bandwidth / times out.

tabulon commented

2024-04-04 14:12:53 +00:00

Sure @fnux, I would be glad.

@nico said he was starting the review last week, but perhaps he got caught up in the mean time.
@nico, what say you?

Meanwhile, I took a look at the changes myself and spotted a few things that could be improved.

On the documentation side, I had probably taken too much liberty, both in terms of tone and content.
Please let me know what you folks think about that. Will happily trim down if needed.
In terms of code, util/filesystem.py could be improved or else reorganized to expose less API, especially
ensure_file_permissions(), whose name promises more than what is actually implemented.

Sure @fnux, I would be glad. @nico said he was starting the review last week, but perhaps he got caught up in the mean time. @nico, what say you? Meanwhile, I took a look at the changes myself and spotted a few things that could be improved. - On the documentation side, I had probably taken too much liberty, both in terms of tone and content. Please let me know what you folks think about that. Will happily trim down if needed. - In terms of code, `util/filesystem.py` could be improved or else reorganized to expose less API, especially `ensure_file_permissions()`, whose name promises more than what is actually implemented.

Sign in to join this conversation.