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

Correction of error message and documentation #504

Merged
merged 21 commits into from
Oct 23, 2024
Merged

Conversation

TomRicci
Copy link
Contributor

There were an issue with the error message when trying to add a server with --protocol scpdown or --protocol scpup (instead of the correct --protocol scpdownload or --protocol scpupload) :

The protocol 'scpup' is not supported, expected either scpup, scpdown, sftp or rsync

Similarly the documentation was wrong on the correct syntaxe for those kind of calls.

@speed47
Copy link
Collaborator

speed47 commented Oct 21, 2024

Hey, thanks for looking into this!

When you modify the code, ensure to run a pass of lint, so that the formatting is fixed if needed, the doc is regenerated, etc. You can do this just by running:
./docker/devenv/run-tool.sh lint

(more details at https://ovh.github.io/the-bastion/development/setup.html#available-tools)

@TomRicci
Copy link
Contributor Author

Hi Stéphane,
From what i'm understanding, the errors concern files I didn't modify in my MR :

*** Checking perl tidiness
## Please see file bin/shell/osh.pl.ERR
## Please see file lib/perl/OVH/Bastion.pm.ERR
## Please see file lib/perl/OVH/Result.pm.ERR
## Please see file lib/perl/OVH/Bastion/ssh.inc.ERR
# cat bin/shell/osh.pl.ERR
Perltidy version is 20220613

bin/shell/osh.pl: Begin Error Output Stream
1977: =cut starts a pod section .. this can fool pod utilities.

Similarly, the lint process seems to change parts I didn't modify, for example :

--- a/bin/plugin/restricted/accountAddPersonalAccess
+++ b/bin/plugin/restricted/accountAddPersonalAccess
@@ -184,15 +184,15 @@ osh_info "Can't verify whether $account\'s personal key has been installed to th
 
 my @command = qw{ sudo -n -u allowkeeper -- /usr/bin/env perl -T };
 push @command, $OVH::Bastion::BASEPATH . '/bin/helper/osh-accountModifyPersonalAccess';
-push @command, '--target', 'any';
-push @command, '--action', 'add';
-push @command, '--account', $account;
-push @command, '--ip', $ip;
-push @command, '--user', $user if $user;
-push @command, '--port', $port if $port;
-push @command, '--force-key', $forceKey if $forceKey;
+push @command, '--target',         'any';
+push @command, '--action',         'add';
+push @command, '--account',        $account;
+push @command, '--ip',             $ip;
+push @command, '--user',           $user          if $user;
+push @command, '--port',           $port          if $port;
+push @command, '--force-key',      $forceKey      if $forceKey;
 push @command, '--force-password', $forcePassword if $forcePassword;
-push @command, '--ttl', $ttl if $ttl;
-push @command, '--comment', $comment if $comment;
+push @command, '--ttl',            $ttl           if $ttl;
+push @command, '--comment',        $comment       if $comment;
 
 osh_exit OVH::Bastion::helper(cmd => \@command);

Can you take give me a hint on what I need to do please ?

@TomRicci
Copy link
Contributor Author

Sorry about that, my browser didn't show the helpfull message in the check's result. I don't understand why the same check runing in a docker on my machine got errors on other files, though.

@speed47
Copy link
Collaborator

speed47 commented Oct 21, 2024

That's strange, because I can't seem to reproduce: I checked out your branch, ran ./docker/devenv/run-tool.sh lint on it, and only the oneline change in ACL.pm you've just done above was found.

The run-tool does spawn a docker with a precise version so that perlcritic/perltidy are under a given fixed version for everyone, to ensure we all get the same results. In this case, it's the version shipped with Ubuntu 20.04:

./docker/devenv/run-tool.sh run perltidy --version
This is perltidy, v20190601 

And not the 20220613 version you seem to have!

@speed47
Copy link
Collaborator

speed47 commented Oct 21, 2024

Checks seem to pass now, you just need to update the generated documentation, though (part of it is generated from the help of the plugins you fixed): ./docker/devenv/run-tool.sh doc will do the trick :)

@TomRicci
Copy link
Contributor Author

TomRicci commented Oct 21, 2024

The run-tool does spawn a docker with a precise version so that perlcritic/perltidy are under a given fixed version for everyone, to ensure we all get the same results. In this case, it's the version shipped with Ubuntu 20.04:

./docker/devenv/run-tool.sh run perltidy --version
This is perltidy, v20190601 

And not the 20220613 version you seem to have!

OK this make more sense to me now, I took a shortcut and directly ran the command /opt/bastion/bin/dev/perl-tidy.sh test in the docker I have on my machine (since docker inside docker doesn't seem to be a good idea to me ^^).

@speed47
Copy link
Collaborator

speed47 commented Oct 23, 2024

Note that the modifications you made in the tests were not really required, as the old parameters still work for backwards compatibility, hence why the tests did pass correctly. But we can keep it that way still :)

@speed47 speed47 added the tests:short Launch short tests through GitHub Actions label Oct 23, 2024
@speed47 speed47 merged commit 05236c1 into ovh:master Oct 23, 2024
9 checks passed
@TomRicci
Copy link
Contributor Author

Merci Stéphane !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests:short Launch short tests through GitHub Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants