action #176862
closedcoordination #154768: [saga][epic][ux] State-of-art user experience for openQA
coordination #176487: [epic] Up-to-date Perl stack
[beginner][easy] Use Feature::Compat::Try in our code - openQA size:S
0%
Description
Motivation¶
Like #176475 but for openQA
Acceptance criteria¶
- AC1: Automatic checks prevent the use of Try::Tiny in openQA
- AC2: All basic
eval { … }; if ($@) …
is replaced bytry { … } catch ($e) { }
in openQA - AC3:: Test::Fatal is not used, and Test::Exception is only used implicitly
Suggestions¶
- See what was done with https://github.com/os-autoinst/os-autoinst/pull/2650 to apply the same for openQA
- See what was done with https://github.com/os-autoinst/os-autoinst/pull/2663 to apply the same for openQA
- Continue with https://github.com/os-autoinst/openQA/pull/6166, create separate commits for each corresponding style change
Updated by okurz about 1 month ago
- Copied from action #176475: Use Feature::Compat::Try in our code - os-autoinst size:S added
Updated by ybonatakis 27 days ago
- Status changed from New to In Progress
- Assignee set to ybonatakis
Updated by ybonatakis 27 days ago
https://github.com/os-autoinst/openQA/pull/6183 (wip)
just submitted what have been done so far. not ready for review
Updated by ybonatakis 25 days ago
https://github.com/os-autoinst/openQA/pull/6183 postponed.
Additional PRs as part of this ticket:
https://github.com/os-autoinst/openQA/pull/6191 (already merged)
https://github.com/os-autoinst/openQA/pull/6189 (already merged)
https://github.com/os-autoinst/openQA/pull/6193
Updated by ybonatakis 24 days ago
- Subject changed from [beginner][easy] Use Feature::Compat::Try in our code - openQA size:S to [beginner][easy] Use Feature::Compat::Try in our code - openQA
- Description updated (diff)
Another one upon all the previous merged PR
https://github.com/os-autoinst/openQA/pull/6194
Updated by okurz 23 days ago
- Subject changed from [beginner][easy] Use Feature::Compat::Try in our code - openQA to [beginner][easy] Use Feature::Compat::Try in our code - openQA size:S
- Description updated (diff)
@ybonatakis please be more careful with ticket updates. I recovered the deleted AC3 and subject addition
Updated by livdywan 20 days ago
Just to clarify the state https://github.com/os-autoinst/openQA/pull/6183 is being updated now
Updated by ybonatakis 19 days ago
Splitted commits (from https://github.com/os-autoinst/openQA/pull/6183) into
https://github.com/os-autoinst/openQA/pull/6211
https://github.com/os-autoinst/openQA/pull/6212
PR6212 is not required after all but nice to have??!
Moreover the https://github.com/os-autoinst/openQA/pull/6183 doesnt replace Try::Tiny completely as we decided to handle some modules separated
Updated by ybonatakis 19 days ago
removed t/deploy.t also from https://github.com/os-autoinst/openQA/pull/6183. it creates confusion
Updated by ybonatakis 19 days ago
- Assignee changed from ybonatakis to tinita
Assigned to Tina as I am blocked by https://app.circleci.com/pipelines/github/os-autoinst/openQA/15898/workflows/41b73103-3190-4845-b95f-c4051b4feb4f/jobs/152992/parallel-runs/0/steps/0-108 and she is working on a fix already
Updated by tinita 19 days ago
Temporarily taking over with https://github.com/os-autoinst/openQA/pull/6206 coverage: Avoid B::Deparse warning with Syntax::Keyword::Try::Deparse
Updated by tinita 19 days ago
- Assignee changed from tinita to ybonatakis
https://github.com/os-autoinst/openQA/pull/6206 merged, other PRs can be rebased, assigning back
Updated by ybonatakis 18 days ago
I did refactor the lib/OpenQA/WebAPI/Controller/API/V1/JobTemplate.pm and tests worked. but I am not sure I have the best solution.
I will think a bit about it before submit.
Updated by ybonatakis 18 days ago
Oli it would be nice to mention https://github.com/os-autoinst/openQA/pull/6214 here as this one is still in progress.
Updated by okurz 18 days ago
ybonatakis wrote in #note-19:
Oli it would be nice to mention https://github.com/os-autoinst/openQA/pull/6214 here as this one is still in progress.
sure, it's related. I mentioned the relation in github PRs as well as in meetings. So how about those separate PRs we discussed for the files you saw problematic so that we can offer specific help for each?
Updated by ybonatakis 17 days ago
Updated by ybonatakis 17 days ago
https://github.com/os-autoinst/openQA/pull/6228
TODO:
- apply check
- modify test which uses Try::Tiny
- not sure but remove try::tiny from deps
Updated by ybonatakis 13 days ago
https://github.com/os-autoinst/openQA/pull/6242
is one of the last tasks required to close this ticket.
I refactored deploy.t but I havent applied the check yet
Updated by okurz 13 days ago
https://github.com/os-autoinst/openQA/pull/6242 merged. Anything else left?
Updated by ybonatakis 13 days ago
- Status changed from In Progress to Feedback
The question is should I remove the Try::Tiny from dependencies.yaml now? or is there any reason we need this. This is the only thing I know before I close the ticket
Updated by ybonatakis 12 days ago
tinita wrote in #note-26:
Yes, it can be removed from the dependencies
Updated by tinita 12 days ago · Edited
Another part od what was done in os-autoinst was to replace block eval
with Test::Exception. Do you think this should be done as part of this ticket as well?
edit: oh, actually AC2 talks about eval.
So i would say eval should be replaced with try/catch, and in tests it should be replaced with Test::Exception (there can be cases where in tests you would also use try/catch)
Updated by ybonatakis 12 days ago
- Status changed from Feedback to In Progress
just push https://github.com/os-autoinst/openQA/pull/6251 but there are more changes to come
Updated by ybonatakis 10 days ago
Updated by ybonatakis 10 days ago
- Status changed from In Progress to Feedback
https://github.com/os-autoinst/openQA/pull/6265
hopefully the last PR
Updated by ybonatakis 9 days ago
And another one https://github.com/os-autoinst/openQA/pull/6267
Updated by okurz 9 days ago
https://github.com/os-autoinst/openQA/pull/6265 merged. Do we have all style checks in place?
Updated by ybonatakis 7 days ago
- Status changed from Feedback to Resolved
okurz wrote in #note-36:
https://github.com/os-autoinst/openQA/pull/6265 merged. Do we have all style checks in place?
Yes, I would say the ones related to this ticket are in place https://github.com/os-autoinst/openQA/blob/master/t/01-style.t
Updated by tinita 7 days ago
There was an unanswered question in https://github.com/os-autoinst/openQA/pull/6263#discussion_r1985237522
Updated by ybonatakis 7 days ago
https://github.com/os-autoinst/openQA/pull/6274
tinita wrote in #note-38:
There was an unanswered question in https://github.com/os-autoinst/openQA/pull/6263#discussion_r1985237522