#2991 PR `merged`: ISO OUTPUT Improvements¶

Labels: enhancement, fixed / solved / done

codefritzel opened issue at 2023-05-24 11:10:¶

Relax-and-Recover (ReaR) Pull Request Template¶

Please fill in the following items before submitting a new pull request:

Pull Request Details:¶

Type: Enhancement
Impact: Low
How was this pull request tested?
manually on Ubu2204
Brief description of the changes in this pull request:

The PR implements the following things:

Restrict RESULT_FILES upload to specific files using OUTPUT_URL_FILES_FILTER filter e.g. only iso files
Username OUTPUT_LFTP_USERNAME and Password OUTPUT_LFTP_PASSWORD can now be given to lftp for authentication at the OUTPUT_URL

What do you think about converting lftp completely to the heredoc method?

lftp "$OUTPUT_URL <<EOF
$OUTPUT_LFTP_OPTIONS
mkdir -fp ${path}
mput ${RESULT_FILES[*]}
EOF

~~Then I could make the code a bit simpler (I wanted to keep the original code for the moment)~~ See b88e1a0

The crendentials are also hidden like #2986 or #2985

jsmeix commented at 2023-05-25 11:57:¶

@rear/contributors
could you also have a look here to be on the safe side
because I may have missed something.

jsmeix commented at 2023-05-31 11:41:¶

@codefritzel
because you did some changes recently
please tell me when you are finished
so I could merge it - unless objections appear.

codefritzel commented at 2023-05-31 13:58:¶

I made a few simplifications to the code and did a rebase. It can merge now, if the changes are OK.

jsmeix commented at 2023-06-02 10:39:¶

It seems after it was merged there is a new
https://github.com/rear/rear/pull/2991#pullrequestreview-1456886332

github-code-scanning bot found potential problems 2 hours ago

What is this?
Did this info already exist while it was not yet merged?

schlomo commented at 2023-06-02 10:49:¶

I'll take a deeper look next week, I also need to get used to our new shell check tooling.

jsmeix commented at 2023-06-02 10:51:¶

In
https://github.com/rear/rear/security/code-scanning/11309
I found

pcahyna closed this as false positive Jun 2, 2023
Glob matching is actually intended here, right?
Codacy seems to have overzealous settings,
as Differential ShellCheck does not complain.

So it seems that those "github-code-scanning bot"
implements plain ShellCheck which complains way too often
in contrast to our carefully setup Differential ShellCheck.

Ugh!
Seems to be fighting against dumb automated tools :-(

pcahyna commented at 2023-06-02 11:11:¶

@schlomo @jsmeix 3cb64f1262eac953b78eec515b1ca561aa1c5b6b

pcahyna commented at 2023-06-02 11:19:¶

What is this?
Did this info already exist while it was not yet merged?

I have not seen it, but the branch codefritzel:output_improvements already had Codacy configured (because it was branched after https://github.com/rear/rear/commit/3cb64f1262eac953b78eec515b1ca561aa1c5b6b) . I must say, a tool that complains about code problems in a PR only after you merge the PR is of rather suboptimal usefulness, if that's what was happening here ...

pcahyna commented at 2023-06-02 11:36:¶

@schlomo by the way, is there a reason why you merge the current state into the PR branch ( Merge branch 'master' into output_improvements e9538f9) before merging the PR? IMO it is unnecessary, it just introduces one merge commit more and thus clutters the history a bit.

jsmeix commented at 2023-06-02 11:41:¶

@pcahyna
thank you for your analysis!

Phew!
When it is under our control then all is OK
because then we can adapt it as we need it.
I had the fear that it could be something that
GitHub does automatically "for us to help us".

I agree with you about its current usefulness.
I think a bot that reports problems in a PR after the merge
behaves more annoying than actually helpful in practice,
perhaps better than nothing, but not developer friendly.

gdha commented at 2023-06-02 12:28:¶

@jsmeix @schlomo @pcahyna When you do not like Codacy Security Scans then just delete the workflow file https://github.com/rear/rear/blob/master/.github/workflows/codacy.yml

jsmeix commented at 2023-06-02 12:36:¶

Since we know what it is and that it is under our control
there is no need to rush.

I did not yet read the details in
https://github.com/codacy/codacy-analysis-cli-action
and
https://github.com/codacy/codacy-analysis-cli

Perhaps the Codacy Security Scans provide
something additional or something different
than our Differential ShellCheck already does
which makes the Codacy Security Scans useful for us?

jsmeix commented at 2023-06-02 12:55:¶

At first glance I don't find how 'shellcheck' could be configured.

https://github.com/codacy/codacy-analysis-cli
reads (excerpt):

Local configuration

To perform certain advanced configurations,
Codacy allows to create a configuration file.
Check our documentation
https://support.codacy.com/hc/en-us/articles/115002130625-Codacy-Configuration-File
for more details.

The link therein redirects to
https://docs.codacy.com/repositories-configure/codacy-configuration-file/
which lists the tool names which are called by Codacy
where in particular 'shellcheck' is listed but then there is
no "Tool-specific configuration" described for shellcheck.

So from my current point of view it seems that
Codacy simply calls plain 'shellcheck', cf. my above
https://github.com/rear/rear/pull/2991#issuecomment-1573535188

If calling plain 'shellcheck' is really all
what the Codacy Security Scans (with plural 's')
do for ReaR then we do not need them because
our own Differential ShellCheck setup is better.

pcahyna commented at 2023-06-02 14:05:¶

I have a theory what went wrong here:

the branch had Codacy configured, but an older version.
https://github.com/rear/rear/pull/3001 got merged to master, updating Codacy
master got merged to this branch at 10:17 AM, bringing updated Codacy
this branch got merged to master at 10:18 AM
new Codacy got triggered in the meantime on the first merge
it reported the result at 10:21 AM

The key hypothesis is that old Codacy would not have reported the problem, but new one does (not sure whether this is true). If it is true, next time we will see the reports already in the PR and not after merge.

pcahyna commented at 2023-06-02 14:06:¶

I would still prefer to turn off ShellCheck feature in Codacy, if that's possible without turning off Codacy entirely (and if there are other features that Codacy offers and they are useful).

jsmeix commented at 2023-06-06 14:10:¶

In my current https://github.com/rear/rear/pull/3006
I see those Codacy things

Codacy Security Scan / Codacy Security Scan (pull_request)
...
Code scanning results / Checkov (reported by Codacy)
Code scanning results / Markdownlint (reported by Codacy)
Code scanning results / Pmd (reported by Codacy)
Code scanning results / Remark-lint (reported by Codacy)
Code scanning results / Shellcheck (reported by Codacy)
Code scanning results / Spectral (reported by Codacy)

So it seems there are other features that Codacy offers
but currently I have no idea what they do (except Shellchek)
nor whether or not they are meaningful or even useful for us.

[Export of Github issue for rear/rear.]

#2991 PR merged: ISO OUTPUT Improvements¶