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

Replace Try::Tiny from remaining OpenQA modules #6228

Merged
merged 1 commit into from
Mar 3, 2025

Conversation

b10n1k
Copy link
Contributor

@b10n1k b10n1k commented Feb 28, 2025

@b10n1k b10n1k force-pushed the feature/replace_old_try branch from 3488e33 to 63478e0 Compare February 28, 2025 11:08
@b10n1k b10n1k force-pushed the feature/replace_old_try branch 3 times, most recently from af5e61b to 6d58440 Compare February 28, 2025 11:59
@okurz
Copy link
Member

okurz commented Feb 28, 2025

Due to the multiple comments I suggest to create a separate PR for just 979e2d1 which should be mergeable easily

@b10n1k b10n1k force-pushed the feature/replace_old_try branch 4 times, most recently from 65f77f9 to d31c30d Compare February 28, 2025 15:34
@b10n1k b10n1k force-pushed the feature/replace_old_try branch from d31c30d to 7a55a53 Compare February 28, 2025 16:15
@b10n1k
Copy link
Contributor Author

b10n1k commented Feb 28, 2025

Due to the multiple comments I suggest to create a separate PR for just 979e2d1 which should be mergeable easily

#6233
I thought I could resolve the problem with the CI faster and due to the fast reaction to the comments. but I had no luck

@b10n1k b10n1k force-pushed the feature/replace_old_try branch from 7a55a53 to 7785c13 Compare February 28, 2025 16:30
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/from/in/

@b10n1k b10n1k force-pushed the feature/replace_old_try branch from 7785c13 to f035983 Compare February 28, 2025 23:33
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.01%. Comparing base (99e9133) to head (998cdde).
Report is 9 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6228   +/-   ##
=======================================
  Coverage   99.01%   99.01%           
=======================================
  Files         398      398           
  Lines       40013    40013           
=======================================
  Hits        39619    39619           
  Misses        394      394           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@b10n1k b10n1k force-pushed the feature/replace_old_try branch from f035983 to 374caec Compare March 1, 2025 07:35
@b10n1k b10n1k removed the not-ready label Mar 1, 2025
@perlpunk
Copy link
Contributor

perlpunk commented Mar 2, 2025

See #6238 for an alternative.
I think most of the return statements in the try are actually intended to return from the sub.

@b10n1k
Copy link
Contributor Author

b10n1k commented Mar 3, 2025

See #6238 for an alternative. I think most of the return statements in the try are actually intended to return from the sub.

is this as a reply to #6228 (comment)?
Maybe it better to close this PR first and proceed to close the task on the scope of the ticket and then improve those concerns.

@okurz
Copy link
Member

okurz commented Mar 3, 2025

I already approved #6238 so if you approve as well you can just rebase on top of it. However other files are not affected so you can go ahead and change all files and bring in the style check

@b10n1k
Copy link
Contributor Author

b10n1k commented Mar 3, 2025

I already approved #6238 so if you approve as well you can just rebase on top of it. However other files are not affected so you can go ahead and change all files and bring in the style check

Why? this is out of scope of the ticket and if I accept it feels another iterations of changes and reviews? When I comment Ionly wanted to bring it up to attention. Can you finish with one task before force further changes?

@perlpunk
Copy link
Contributor

perlpunk commented Mar 3, 2025

Maybe it better to close this PR first and proceed to close the task on the scope of the ticket and then improve those concerns.

Well, I see my PR in scope of the ticket.
The worker_status code is a bit complicated, and I am even not 100% sure myself if I didn't break anything with my PR.
Your suggestions for further changes seemed to go in the wrong direction.
Now you can concentrate on the try/catch only.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There is at least one point that @perlpunk brought up and already requested changes on. Assuming that any related changes would be trivial we can keep my approval.

@b10n1k b10n1k force-pushed the feature/replace_old_try branch from 374caec to 2e5bd17 Compare March 3, 2025 10:15
@perlpunk perlpunk dismissed their stale review March 3, 2025 10:41

original issue resolved

Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think #6238 first would be easier for everyone, but ok

Copy link
Contributor

mergify bot commented Mar 3, 2025

This pull request is now in conflicts. Could you fix it? 🙏

@okurz
Copy link
Member

okurz commented Mar 3, 2025

#6238 is merged so please rebase on top. @b10n1k I am sure @perlpunk is happy to help to resolve any non-trivial conflicts

@b10n1k b10n1k force-pushed the feature/replace_old_try branch 2 times, most recently from c5c8b52 to f0495fe Compare March 3, 2025 12:25
Another patch of the remaining leftover modules which didnt migrated to
Feature::Compat::Try.

https://progress.opensuse.org/issues/176862
@b10n1k b10n1k force-pushed the feature/replace_old_try branch from f0495fe to 998cdde Compare March 3, 2025 13:03
@okurz okurz merged commit 2d84d3a into os-autoinst:master Mar 3, 2025
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants