-
Notifications
You must be signed in to change notification settings - Fork 208
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
Remove unused 'dsn' function and its usage #4928
Remove unused 'dsn' function and its usage #4928
Conversation
d580029
to
b912b26
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
b912b26
to
c8d031f
Compare
c8d031f
to
966b11c
Compare
966b11c
to
e5501d7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4928 +/- ##
==========================================
+ Coverage 98.49% 98.50% +0.01%
==========================================
Files 394 394
Lines 38683 38678 -5
==========================================
Hits 38099 38099
+ Misses 584 579 -5 ☔ View full report in Codecov by Sentry. |
e5501d7
to
5ffe6e4
Compare
Removed superfluous |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks wrong. At least the commit message is misleading (as I only see a function being removed but not moved/inlined).
Tests did not cover the 'dsn' function anyway. Verified by calling ``` env OPENQA_BASEDIR=$PWD/t/data OPENQA_DATABASE=test script/openqa-gru ``` locally.
5ffe6e4
to
b1da48e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't need this way of specifying credentials anymore then.
There is an issue in production due to this:
|
The good thing is that we saw this in docker-compose tests. I wonder if I can change the docker-compose tests to actually test the branch related to a pull request as we build packages in there - if that's not taking too long. |
That would mean the execution of the docker-compose tests needs to happen after the OBS checks have built and published the packages. Not sure whether we can easily chain OBS checks and docker-compose tests as they are triggered/executed in a completely different way. |
In the helm chart tests (which also failed), it pulls the container images from OBS. The docker-compose test does the build on its own. The test is calling openQA/tools/test_containers_compose Line 30 in 4389076
Another possible solution could be a new Dockerfile to build openQA container locally from git (but that would be duplicating the work OBS does for us) or as was already mentioned, wait for OBS, but even in that case we need to modify the Dockerfile to use the packages from the correct branch and not the released ones from our devel repo. |
ok, nevermind the docker-compose ideas. Now second try for the original change but with full test coverage of the changed code: |
Tests did not cover the 'dsn' function anyway.