Validate YAML on Bitbucket Server & Refactor the Validation #1846
Conversation
|
/ok-to-test |
|
thanks for the contribution! cc @zakisk |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1846 +/- ##
==========================================
+ Coverage 65.41% 65.78% +0.37%
==========================================
Files 178 178
Lines 13757 13839 +82
==========================================
+ Hits 8999 9104 +105
+ Misses 4154 4122 -32
- Partials 604 613 +9 ☔ View full report in Codecov by Sentry. |
|
|
||
| func ValidateYaml(content []byte, filename string) error { | ||
| var validYaml any | ||
| if err := yaml.Unmarshal(content, &validYaml); err != nil { |
There was a problem hiding this comment.
@tricktron why did you choose "k8s.io/apimachinery/pkg/util/yaml" over "gopkg.in/yaml.v2"?, is there any specific reason?
There was a problem hiding this comment.
@tricktron why did you choose "k8s.io/apimachinery/pkg/util/yaml" over "gopkg.in/yaml.v2"?, is there any specific reason?
@zakisk Ups, I just reused the k8s.io/apimachinery/pkg/util/yaml during the refactoring from this pr. But now that you mention it, I see that k8s.io/apimachinery/pkg/util/yaml is only used once in provider/github and the gopkg.in/yaml.v2 dependency is used more often.
Therfore, it might make sense to only use gopkg.in/yaml.v2 so that we can ditch k8s.io/apimachinery/pkg/util/yaml. What do you think?
There was a problem hiding this comment.
@zakisk I switched from k8s.io/apimachinery/pkg/util/yaml to gopkg.in/yaml.v2 for yaml validation.
There was a problem hiding this comment.
i think if i rem ok we need gopkg.yaml for yaml error validation in that pacakge, all other we use k8 machinery yaml because it remove a lot of extra stuff when reading/writting a yaml
i think the e2e test fails on that
There was a problem hiding this comment.
@tricktron can you please take at this line, this is the reason E2E is failing.
|
/test go-testing |
|
/retest |
instead of k8s.io/apimachinery/pkg/util/yaml.
tricktron
force-pushed
the
f-print-bitbucket-yaml-error
branch
from
715af92 to
600da7e
Compare
|
@zakisk I don´t know why the e2e tests failed (I have not changed them). I rebased on main to get the latest commits and pushed again to run the tests. |
it happens sometimes, let's wait for this new run! |
|
@tricktron oh, I missed @chmouel 's comment. |
match wrapped errors of gopkg.in/yaml.v2 instead of errors of k8s.io/apimachinery/pkg/util/yaml.
tricktron
force-pushed
the
f-print-bitbucket-yaml-error
branch
from
2db04da to
4a01a2f
Compare
|
Thank you for your contribution ! @tricktron |
Changes
The main goal of this pull request is to better surface error logs to the pipeline as code developer on Bitbucket Server: https://issues.redhat.com/browse/SRVKP-6767.
Essentially, this pull request does three things in three commits:
Submitter Checklist
📝 Please ensure your commit message is clear and informative. For guidance on crafting effective commit messages, refer to the How to write a git commit message guide. We prefer the commit message to be included in the PR body itself rather than a link to an external website (ie: Jira ticket).
♽ Before submitting a PR, run make test lint to avoid unnecessary CI processing. For an even more efficient workflow, consider installing pre-commit and running pre-commit install in the root of this repository.
✨ We use linters to maintain clean and consistent code. Please ensure you've run make lint before submitting a PR. Some linters offer a --fix mode, which can be executed with the command make fix-linters (ensure markdownlint and golangci-lint tools are installed first).
📖 If you're introducing a user-facing feature or changing existing behavior, please ensure it's properly documented.
🧪 While 100% coverage isn't a requirement, we encourage unit tests for any code changes where possible.
🎁 If feasible, please check if an end-to-end test can be added. See README for more details.
🔎 If there's any flakiness in the CI tests, don't necessarily ignore it. It's better to address the issue before merging, or provide a valid reason to bypass it if fixing isn't possible (e.g., token rate limitations).