Project

General

Profile

Actions
Copy link

action #176862

closed

coordination #127031: [saga][epic] openQA for SUSE customers

coordination #168127: [epic] Up-to-date Perl stack

[beginner][easy] Use Feature::Compat::Try in our code - openQA size:S

Added by okurz 2 months ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Low
Assignee:
ybonatakis
Category:
Feature requests
Target version:
Ready
Start date:
Due date:
% Done:

0%

Estimated time:

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 by try { … } catch ($e) { } in openQA
  • AC3:: Test::Fatal is not used, and Test::Exception is only used implicitly

Suggestions

Related issues 1 (0 open — 1 closed)

Copied from openQA Project (public) - action #176475: Use Feature::Compat::Try in our code - os-autoinst size:SResolvedokurz2025-02-03

Actions
Actions
Copy link
 #1

Updated by okurz 2 months ago

  • Copied from action #176475: Use Feature::Compat::Try in our code - os-autoinst size:S added
Actions
Copy link
 #2

Updated by okurz 2 months ago

  • Description updated (diff)
  • Status changed from Blocked to New

#176475 successfully resolved.

Actions
Copy link
 #3

Updated by okurz 2 months ago

  • Description updated (diff)
  • Assignee deleted (okurz)
  • Target version changed from Tools - Next to Ready
Actions
Copy link
 #4

Updated by okurz 2 months ago

  • Subject changed from Use Feature::Compat::Try in our code - openQA to [beginner][easy] Use Feature::Compat::Try in our code - openQA
Actions
Copy link
 #5

Updated by okurz 2 months ago

  • Priority changed from Normal to Low
Actions
Copy link
 #6

Updated by ybonatakis 2 months ago

  • Status changed from New to In Progress
  • Assignee set to ybonatakis
Actions
Copy link
 #7

Updated by ybonatakis 2 months ago

https://github.com/os-autoinst/openQA/pull/6183 (wip)
just submitted what have been done so far. not ready for review

Actions
Copy link
 #9

Updated by livdywan 2 months 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)
Actions
Copy link
 #10

Updated by ybonatakis 2 months 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

Actions
Copy link
 #11

Updated by okurz 2 months 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

Actions
Copy link
 #12

Updated by livdywan about 2 months ago

Just to clarify the state https://github.com/os-autoinst/openQA/pull/6183 is being updated now

Actions
Copy link
 #13

Updated by ybonatakis about 2 months 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

Actions
Copy link
 #14

Updated by ybonatakis about 2 months ago

removed t/deploy.t also from https://github.com/os-autoinst/openQA/pull/6183. it creates confusion

Actions
Copy link
 #15

Updated by ybonatakis about 2 months ago

  • Assignee changed from ybonatakis to tinita
Actions
Copy link
 #16

Updated by tinita about 2 months ago

Temporarily taking over with https://github.com/os-autoinst/openQA/pull/6206 coverage: Avoid B::Deparse warning with Syntax::Keyword::Try::Deparse

Actions
Copy link
 #17

Updated by tinita about 2 months ago

  • Assignee changed from tinita to ybonatakis

https://github.com/os-autoinst/openQA/pull/6206 merged, other PRs can be rebased, assigning back

Actions
Copy link
 #18

Updated by ybonatakis about 2 months 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.

Actions
Copy link
 #19

Updated by ybonatakis about 2 months ago

Oli it would be nice to mention https://github.com/os-autoinst/openQA/pull/6214 here as this one is still in progress.

Actions
Copy link
 #20

Updated by okurz about 2 months 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?

Actions
Copy link
 #22

Updated by ybonatakis about 2 months 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
Actions
Copy link
 #23

Updated by ybonatakis about 2 months 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

Actions
Copy link
 #24

Updated by okurz about 2 months ago

Actions
Copy link
 #25

Updated by ybonatakis about 2 months 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

Actions
Copy link
 #26

Updated by tinita about 2 months ago

Yes, it can be removed from the dependencies

Actions
Copy link
 #27

Updated by ybonatakis about 2 months ago

tinita wrote in #note-26:

Yes, it can be removed from the dependencies

https://github.com/os-autoinst/openQA/pull/6245

Actions
Copy link
 #28

Updated by tinita about 2 months 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)

Actions
Copy link
 #29

Updated by ybonatakis about 2 months 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

Actions
Copy link
 #30

Updated by livdywan about 2 months ago

As suggested in the call, we should wrap this up by Friday and bring up any open points in the collab session. Otherwise this needs to be re-estimated.

Actions
Copy link
 #31

Updated by livdywan about 2 months ago · Edited

All basic eval { … }; if ($@) … is replaced by try { … } catch ($e) { } in openQA

Conversely eval { } w/o $@ should be left as-is. Confirmed on the call.

Actions
Copy link
 #34

Updated by ybonatakis about 2 months ago

  • Status changed from In Progress to Feedback
Actions
Copy link
 #36

Updated by okurz about 2 months ago

https://github.com/os-autoinst/openQA/pull/6265 merged. Do we have all style checks in place?

Actions
Copy link
 #37

Updated by ybonatakis about 2 months 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

Actions
Copy link
 #40

Updated by okurz about 1 month ago

  • Parent task changed from #176487 to #168127
Actions
Copy link

Also available in: Atom PDF