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

Enlarge visible windows only in fullscreen method #2027

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

rakoenig
Copy link
Contributor

See ticket: poo#109795 for details.

@github-actions
Copy link

github-actions bot commented Apr 12, 2022

Great PR! Please pay attention to the following items before merging:

Files matching consoles/**.pm:

  • Consider running manual verification tests on real hardware, especially for non-qemu backends

This is an automatically generated QA checklist based on modified files

@okurz
Copy link
Member

okurz commented Apr 12, 2022

@rakoenig please see #2027 (comment) and provide a manual verification test where we can see that working in action as we currently don't have automatic CI tests covering that behaviour.

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

Looks good, there's only one typo ("poblems") in the commit message.

- So far this method was determining the window_id of the window
  to resize to fullscreen by limiting the list to 1 entry. This could
  select the wrong window, resulting in problems that
  https://progress.opensuse.org/issues/106514 wants to fix.
- now we use --onlyvisible to select the visible window only.
@codecov
Copy link

codecov bot commented Apr 12, 2022

Codecov Report

Merging #2027 (f0702c0) into master (112ac9e) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2027      +/-   ##
==========================================
+ Coverage   77.53%   77.56%   +0.02%     
==========================================
  Files          70       70              
  Lines        7265     7273       +8     
==========================================
+ Hits         5633     5641       +8     
  Misses       1632     1632              
Impacted Files Coverage Δ
consoles/localXvnc.pm 100.00% <100.00%> (ø)
mmapi.pm 100.00% <0.00%> (ø)
bmwqemu.pm 100.00% <0.00%> (ø)
lockapi.pm 93.40% <0.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 112ac9e...f0702c0. Read the comment docs.

@kalikiana
Copy link
Member

@rakoenig please see #2027 (comment) and provide a manual verification test where we can see that working in action as we currently don't have automatic CI tests covering that behaviour.

@rakoenig Did you see this comment?

@rakoenig
Copy link
Contributor Author

@rakoenig Did you see this comment?

Yes I saw it and I'm desperately trying to make a verification run. But so far what people suggested on the Slack-channel didn't work for me. Running the worker in the container was a complete fail. I'm a bit lost on how to tweak the workers from remote, but looks like there is not much of a documentation or even a tutorial about that stuff.

@okurz
Copy link
Member

okurz commented Apr 21, 2022

@Martchus could you help rakoenig to get this tested?

@Martchus
Copy link
Contributor

Martchus commented Apr 21, 2022

When I remember the chat history correctly, that change was actually tested (by manually logging in on the host and invoking the command manually). Hence I've also already approved the change and would say we can even merge it without further testing.


If we really want to do further testing, I'd probably go for the approach documented here: https://progress.opensuse.org/projects/openqav3/wiki/#Use-a-production-host-for-testing-backend-changes-locally-eg-svirt-powerVM-IPMI-bare-metal-s390x-etc - This is the approach used so far when developing with the svirt backend.

Alternatively, you can also hot-patch an instance. Take it out of production and apply a patch via patch -d /usr/lib/os-autoinst -p1 -i …. To run a test on that specific worker, clone it with the worker-specific WORKER_CLASS setting. You don't need to restart any services on the host for os-autoinst changes to apply. When you're done, revert your changes and bring the worker back.

(All other suggestions were only luring you into uncharted territory. They might be better in the long run but you'll have to figure out details on your own at this point.)

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.

I am ok if the command was tried out on the actual production machines

@mergify mergify bot merged commit 22e982c into os-autoinst:master Apr 21, 2022
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.

None yet

4 participants