Project

General

Profile

action #99663

coordination #80142: [saga][epic] Scale out: Redundant/load-balancing deployments of openQA, easy containers, containers on kubernetes

coordination #99579: [epic][retro] Follow-up to "Published QCOW images appear to be uncompressed"

coordination #99660: [epic] Use more perl signatures in our perl projects

Use more perl signatures - os-autoinst size:M

Added by okurz 8 months ago. Updated 3 days ago.

Status:
Feedback
Priority:
Low
Assignee:
Category:
Feature requests
Target version:
Start date:
2021-10-01
Due date:
% Done:

0%

Estimated time:
Difficulty:

Description

Motivation

See #99660. Use signatures to prevent unintended misuses of function signatures, here for os-autoinst


Related issues

Related to openQA Project - action #102146: Deprecate os-autoinst backend::pvmResolved2021-11-092021-11-26

Related to openQA Project - action #104986: tests incomplete with: auto_review:"backend died: Too many arguments for subroutine.*consoles::vnc_base::get_last_mouse_set":retryResolved2022-01-18

Related to openQA Project - action #104520: Move svirt extract_asset code from os-autoinst-distri-opensuse to os-autoinst/backend/svirt.pm size:M auto_review:"unable to extract assets: Can't call method.+name.+on an undefined value":retryWorkable2021-12-29

Related to openQA Project - action #106654: [ipmi][openqa][vnc] Massive test run failures with 'IO::Socket::INET: connect: Connection refused' due to "Use of uninitialized value.*connect_timeout in addition.*consoles/VNC.pm line 13.*":retryResolved2022-02-11

Related to openQA Project - action #108323: Subroutine consoles::sshVirtsh::has redefined at .../Class/Accessor.pmResolved2022-03-14

Related to openQA Project - action #110983: Wrong signatures auto_review:"Too.*arguments for subroutine":retryResolved2022-05-12

Copied to openQA Project - action #100967: Use more perl signatures - openQA size:MWorkable

History

#1 Updated by okurz 8 months ago

  • Status changed from New to In Progress

#2 Updated by okurz 8 months ago

I had started already here but as I recurringly fail to finish I am splitting this up in multiple PRs for easier testing and planned merges one at a time:

let's see which ones succeed. Based on criticality we can decide which ones to merge first. I suggest to first do low-risk ones like tools and t/, later backends and consoles

EDIT: Updated current status of PRs

#3 Updated by okurz 8 months ago

3 done, 3 to go

#4 Updated by okurz 7 months ago

  • Copied to action #100967: Use more perl signatures - openQA size:M added

#5 Updated by okurz 7 months ago

additional PR for myjsonrpc only: https://github.com/os-autoinst/os-autoinst/pull/1827 - merged

#6 Updated by okurz 6 months ago

#7 Updated by okurz 6 months ago

I now merged https://github.com/os-autoinst/os-autoinst/pull/1790 "use signatures in os-autoinst backend" as it was ready and has approval. I will have alert duty this week so hopefully I would see any fallout :)

#8 Updated by okurz 6 months ago

There was no feedback on the last merged pull request so I assume we are good and can continue with the next, i.e. "consoles"

#9 Updated by cdywan 6 months ago

okurz wrote:

There was no feedback on the last merged pull request so I assume we are good and can continue with the next, i.e. "consoles"

https://github.com/os-autoinst/os-autoinst/pull/1868

#10 Updated by okurz 6 months ago

I am playing it safe and doing the first part of consoles with https://github.com/os-autoinst/os-autoinst/pull/1868 (merged). https://github.com/os-autoinst/os-autoinst/pull/1789 includes the second part. I realized that a good additional test is to execute the openQA full stack test while I have my signatures changes in os-autoinst checked out locally. Seems like maybe the os-autoinst tests do not cover the complete interface for the interactive mode and such so I will test my changes with openQA first.

#11 Updated by okurz 5 months ago

  • Status changed from In Progress to Feedback

I will wait for some more critical os-autoinst related changes before continuing, e.g. waiting for #103422 and #103611

#12 Updated by okurz 4 months ago

  • Status changed from Feedback to In Progress

prepared https://github.com/os-autoinst/os-autoinst/pull/1899 for parts of the consoles and merged. Continuing with consoles in https://github.com/os-autoinst/os-autoinst/pull/1789

#13 Updated by okurz 4 months ago

  • Related to action #104986: tests incomplete with: auto_review:"backend died: Too many arguments for subroutine.*consoles::vnc_base::get_last_mouse_set":retry added

#14 Updated by okurz 4 months ago

  • Related to action #104520: Move svirt extract_asset code from os-autoinst-distri-opensuse to os-autoinst/backend/svirt.pm size:M auto_review:"unable to extract assets: Can't call method.+name.+on an undefined value":retry added

#15 Updated by okurz 4 months ago

  • Status changed from In Progress to Feedback

The PR https://github.com/os-autoinst/os-autoinst/pull/1789 is basically ready but so far nobody dared to merge it due to the lower statement code coverage of consoles::VNC. One simple related change which actually also touches consoles::VNC is https://github.com/os-autoinst/os-autoinst/pull/1934 about the "ikvm" backend. Currently https://app.codecov.io/gh/os-autoinst/os-autoinst/blob/master/consoles/VNC.pm reports 57%. I rebased https://github.com/os-autoinst/os-autoinst/pull/1789 and will see what this will end up with as coverage. Previously the patch coverage in the PR was reported as 67%.

#16 Updated by okurz 3 months ago

surprisngly cdywan merged https://github.com/os-autoinst/os-autoinst/pull/1789 . I assume they missed our discussions and plans. Regardless, I checked jobs on https://openqa.opensuse.org/tests and found no related failures so I guess we are good with that change. I can now continue with https://github.com/os-autoinst/os-autoinst/pull/1773 but that's still a rather big PR so maybe split out more files.

#17 Updated by cdywan 3 months ago

okurz wrote:

surprisngly cdywan merged https://github.com/os-autoinst/os-autoinst/pull/1789 . I assume they missed our discussions and plans. Regardless, I checked jobs on https://openqa.opensuse.org/tests and found no related failures so I guess we are good with that change.

2 approvals, 2 other reviewers, no objections after having brought up concerns with coverage. I sanity-checked also. Typically we merge in such cases. If you clearly didn't want it merged I suggest a blocking review, draft or not-ready label for the future.

I can now continue with https://github.com/os-autoinst/os-autoinst/pull/1773 but that's still a rather big PR so maybe split out more files.

How about consoles+backends and api(mmapi/bmwqemu/distribution) or something along those lines?

#18 Updated by okurz 3 months ago

cdywan wrote:

okurz wrote:

surprisngly cdywan merged https://github.com/os-autoinst/os-autoinst/pull/1789 . I assume they missed our discussions and plans. Regardless, I checked jobs on https://openqa.opensuse.org/tests and found no related failures so I guess we are good with that change.

2 approvals, 2 other reviewers, no objections after having brought up concerns with coverage. I sanity-checked also. Typically we merge in such cases. If you clearly didn't want it merged I suggest a blocking review, draft or not-ready label for the future.

sure, not your fault at all but mine and it seems all turned out well.

I can now continue with https://github.com/os-autoinst/os-autoinst/pull/1773 but that's still a rather big PR so maybe split out more files.

How about consoles+backends and api(mmapi/bmwqemu/distribution) or something along those lines?

yes, that's what I am thinking.

#19 Updated by okurz 3 months ago

  • Related to action #106654: [ipmi][openqa][vnc] Massive test run failures with 'IO::Socket::INET: connect: Connection refused' due to "Use of uninitialized value.*connect_timeout in addition.*consoles/VNC.pm line 13.*":retry added

#20 Updated by okurz 2 months ago

  • Related to action #108323: Subroutine consoles::sshVirtsh::has redefined at .../Class/Accessor.pm added

#22 Updated by okurz 21 days ago

  • Subject changed from Use more perl signatures - os-autoinst to Use more perl signatures - os-autoinst size:M

#23 Updated by okurz 19 days ago

https://github.com/os-autoinst/os-autoinst/pull/2046 for commands.pm . Should wait a day or two for feedback before continuing.

#24 Updated by okurz 14 days ago

  • Status changed from Feedback to In Progress

PR merged, no problems observed. Continuing with https://github.com/os-autoinst/os-autoinst/pull/1773 , now merged as well. Continuing with more files which have received non-signature changes since then and some places which I might have missed and testapi.pm itself as well.

Continuing in https://mysuse.sharepoint.com/:v:/s/HowWeWork/ERL9nT8YLmNOnfjj5G53MfUBE2rFh1BeYA7kNTxhZpC4UQ?e=iYvr7b

#25 Updated by okurz 11 days ago

  • Related to action #110983: Wrong signatures auto_review:"Too.*arguments for subroutine":retry added

#26 Updated by okurz 10 days ago

Brought up in weekly 2022-05-13. I was making a stupid mistake on trying to just add -signatures in one commit and see how that goes but of course that stumbles already over the prototypes used. So I should definitely use both together.

I am welcoming everybody to review https://github.com/os-autoinst/os-autoinst/pull/1696 and help me with some suggestions.

#27 Updated by okurz 7 days ago

https://github.com/os-autoinst/os-autoinst/pull/2060 merged including parts of the above including a syntax check to ensure we have signatures in all perl files, except testapi.pm so far.

#28 Updated by okurz 5 days ago

  • Status changed from In Progress to Feedback

#29 Updated by okurz 3 days ago

As this PR still turns out to be too big to be easily review I am splitting out yet another PR:

Also available in: Atom PDF