OLS-2316 - Adding section about reporting periodic tests#592
OLS-2316 - Adding section about reporting periodic tests#592JoaoFula wants to merge 1 commit intokonflux-ci:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Tekton pipeline for generating weekly reports on periodic integration tests, along with corresponding AsciiDoc documentation. However, the bash script within the pipeline contains several critical command injection vulnerabilities due to improper handling of user-supplied parameters, including direct command substitution of variable content, unquoted variable usage in shell commands, and the use of unquoted heredocs that evaluate user-controlled data. These issues allow for arbitrary code execution within the CI/CD environment. Besides the security issues, the reporting.yaml file contains invalid YAML syntax that will prevent the pipeline from being parsed and a bug in the shell script that will cause an error during execution. There are also high-severity issues related to hardcoded URLs and redundant package installations. Further improvements are suggested to enhance script efficiency. The documentation in reporting-periodic-tests.adoc has been reviewed against the repository's style guide and contains a high-severity issue with a broken link, along with several medium-severity violations of the style guide and an instance of using a deprecated field in an example.
| fi | ||
|
|
||
| # Generate log link (OpenShift Console URL) | ||
| local log_link="https://konflux-ui.apps.stone-prd-rh01.pg1f.p1.openshiftapps.com/ns/$NAMESPACE/applications/$APPLICATION/pipelineruns/$name" |
There was a problem hiding this comment.
Don't hardcore konflux deployment URLs into docs
There was a problem hiding this comment.
or code if this is supposed to be reusable
There was a problem hiding this comment.
So for the log link I should have it depend on a parameter called KONFLUX_UI_URL or something?
MartinBasti
left a comment
There was a problem hiding this comment.
I don't think that this belongs to upstream documentation.
What is the purpose of this for common konflux user? This seems like very specific use case not a general one.
Examples are full of Red Hat specific deployments, that's shouldn't be in doc.
Also, dumping 500 lines script as example doesn't look well. If this is something konflux users desire, then it should be a real well tested task bundle, not a example to be fixed.
|
The idea around this is to have a general pipeline for users to be able to get their test results. Konflux does not do a good job at providing users with a clear way to visualize test results, especially so when it comes to periodic ones (you have to search through your pipeline runs, looking at the time when your pipeline ran and do this for every single day of the week you want to look at). This is a way for users to be able to pull that information and automate it to get said report on their email weekly. Furthermore, I'd argue that automated test reporting when you have a section for automated periodic testing is anything but specific. This should always be something that teams strive to have. Tests and clear visualization of the results for those tests. I agree that this requires a lot of fixing but I still think that having a guideline for anyone wanting to create reporting for their periodic pipelines makes complete sense, especially when visualization of said results is not at all clear. |
Adding section about reporting periodic tests
574f5ff to
8374f16
Compare
Have you filed a konflux bug? |
|
Hey @MartinBasti , I don't think that's a bug report situation. Potentially a feature request and I can definitely put that in. That being said, I do understand you don't want such a big script in the docs. I suggest us moving this script somewhere else and not paste it here as we do in other places in the docs. |
|
@JoaoFula would you consider contributing the proposed pipeline to the https://github.com/konflux-ci/tekton-integration-catalog repository? |
|
@dirgim konflux-ci/tekton-integration-catalog#279 created and tested. We can adapt the docs to mention this pipeline when merged or ignore the docs changes altogether. Up to you |
No description provided.