Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/40e09c3bb85143a189e409881d88df23

• os-autoinst-tox : SUCCESS in 4m 35s
• os-autoinst-perl : FAILURE in 7m 10s

@adamwill @lruzicka , I see "needs tidying" in the log of perl zuul failure. I am not sure what that means. Also, I remember Adamw mentioning something about the template file. I am not sure what that would entail. Please advice :)

Thanks for this, sumantro! tidy is a code linter/style checker for perl; run ./tidy at the top level of the repo. You will need perltidy and perl-Code-TidyAll installed (you can do this from a container if you don't want to pollute your main system).

Maybe I should tweak the CI scripts to explain this.

The templates file is what actually configures things so something will run your test. We can show you how to do that after we review the test itself a bit. @lruzicka , do you want to have a look at this?

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/0c2f40c42f7443b98690c69e33f2cd14

• os-autoinst-tox : SUCCESS in 4m 34s
• os-autoinst-perl : SUCCESS in 7m 32s

Thanks for this, sumantro! tidy is a code linter/style checker for perl; run ./tidy at the top level of the repo. You will need perltidy and perl-Code-TidyAll installed (you can do this from a container if you don't want to pollute your main system).

Maybe I should tweak the CI scripts to explain this.

The templates file is what actually configures things so something will run your test. We can show you how to do that after we review the test itself a bit. @lruzicka , do you want to have a look at this?

Hey @adamwill and @lruzicka , now that zuul is happy and green. I would love to know the reviews on the test itself. I have mostly covered all scenarios.

Thanks! Hopefully @lruzicka can take a look while I'm on PTO, if not I will have a look when I'm back. One thing I noticed on a quick lookover - the arg to assert_script_run is a timeout in seconds. You set it to 300 for a lot of commands. That's quite a long timeout; it means that if the command gets stuck somehow, openQA will wait 300 seconds before giving up and marking the test as failed. We generally use the default timeout for most commands which would be expected to complete quite fast (the default is 90 seconds), and set it to a higher value for things like package installations which can take a while. I'm not sure how long each of those commands takes, but if any of them would be expected to return quite quickly if it's working normally, it might be OK to drop the 300 timeout.

One thing I'm curious about but can't guess is whether we'll actually be able to use assert_script_run for all these commands. To get technical, the way that actually works is it runs the command and redirects its return code - padded with some random characters to avoid collisions - to the serial console. In a separate channel, the worker host is capturing the output of the serial console and it looks for the return code to find out if the command succeeded.

This works as long as it's actually possible to direct the return code to the serial console. For instance, you can't normally use assert_script_run as a regular user, because the user doesn't have the right to write to the serial console; if you need to do that, you have to change the permissions as root first. What I'm curious about is whether we'll have any issues writing to the serial console from within a container, or when starting a container. But I guess we'll have to try it to see :D

Thanks! Hopefully @lruzicka can take a look while I'm on PTO, if not I will have a look when I'm back. One thing I noticed on a quick lookover - the arg to assert_script_run is a timeout in seconds. You set it to 300 for a lot of commands. That's quite a long timeout; it means that if the command gets stuck somehow, openQA will wait 300 seconds before giving up and marking the test as failed. We generally use the default timeout for most commands which would be expected to complete quite fast (the default is 90 seconds), and set it to a higher value for things like package installations which can take a while. I'm not sure how long each of those commands takes, but if any of them would be expected to return quite quickly if it's working normally, it might be OK to drop the 300 timeout.

One thing I'm curious about but can't guess is whether we'll actually be able to use assert_script_run for all these commands. To get technical, the way that actually works is it runs the command and redirects its return code - padded with some random characters to avoid collisions - to the serial console. In a separate channel, the worker host is capturing the output of the serial console and it looks for the return code to find out if the command succeeded.

This works as long as it's actually possible to direct the return code to the serial console. For instance, you can't normally use assert_script_run as a regular user, because the user doesn't have the right to write to the serial console; if you need to do that, you have to change the permissions as root first. What I'm curious about is whether we'll have any issues writing to the serial console from within a container, or when starting a container. But I guess we'll have to try it to see :D

woah! I have worked on the wait time reduction problem. Except for syntax which fetches images of ~200+mb I have explicitly made other "90" . I am waiting for @lruzicka's blessings and advice :D

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/677119cb3267423b9a77ce44958a38c9

• os-autoinst-tox : SUCCESS in 4m 34s
• os-autoinst-perl : SUCCESS in 8m 09s

Hello @sumantrom and @adamwill,

I have not much experience with toolbox, so whenever I am questioning something in my comments, the reason might be that I do not fully understand how the commands should work.

I will comment inline.

Why are you using these extra packages? In the following script, I do not see anything that would not be covered by the testapi package.

This should be investigated more as I have realized, that toolbox is installed by default on Workstation. I believe it could be done in a different way:

1. Check if toolbox is installed.
2. Only install it, if it is not installed.

This can be achieved with checking the return code of the script_run command, see the openQA testapi for more details.

This command would not work in openQA. At least for me, when I use it, it asks for confirmation [y/N]. Now, one of the keys should be pressed, but you are not pressing the key in the script. You either need to send the y key to confirm the command or use the -y option with that command.

The way it is used now, will become stalled and the test will fail in 300 seconds.

The default time out for commands is 90 seconds and for needles, it is 30 seconds. I do not find useful to put it into the script and I personally would avoid it, if the number is the default number anyway. Using assert_script_run "echo Hello!" is completely fine and perfectly readable on the first sight. Reserve the timeout definitions for commands that require longer or shorter period of time.

Now, there is a problem I have here. I have entered a container1 previously and now I am inside it. Do I want to create another container from withing the original one or is this a mistake? Should I rather exit that container, stop it, remove it, and then continue with another container? I have tested manually and it sort of works how it is designed, but somehow I do not see any logic in it. What do you think?

By the way, creating this container in a way it is written also requires confirmation!

This command will never return the expected value, use cat /etc/redhat-release instead.

This command has a typo conatiner instead of container. However, it does not work for removing the image anyway. At least for my installation. To remove the image, I need to pass the image ID, such as 4880eddcb00c or the image name, such as registry.access.redhat.com/ubi9/toolbox:9.1.

This will remove the container if the command succeeds, but we will only find out, that the command succeeded in running, but we will not see, if the container has been deleted. Maybe we should check?

You are logging to a root console, which is fine for assert_script_runs and other script routines that need to write their output to the serial console. If you need to make the serial console writable for a non-root user, you need to make it writable first. See make_serial_writable.

ALso, you can check that you have indeed entered the container and not only rely on the command succeeding.

Why are you using these extra packages? In the following script, I do not see anything that would not be covered by the testapi package.

Can you point out if there are any mandatory packages that I will need to import... I used the podman.pm as base. Podman had all the packages and I assumed that if podman would need them, toolbox may need them as well.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/787681f7cff348cc940b0965c03774f8

• os-autoinst-tox : SUCCESS in 5m 30s
• os-autoinst-perl : FAILURE in 6m 55s

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/590eae706e7245628ea545bc65b82e13

• os-autoinst-tox : SUCCESS in 4m 14s
• os-autoinst-perl : FAILURE in 7m 54s

@lruzicka I have fixed mostly taken into account all your suggestions and made changes. I am still waiting on your review and the packages that must be imported to make the test case executable.
I have fixed the execution lineup and now this should be pretty doable. Can we run this somewhere and see what is the actual outcome?
If I recall what @adamw mentioned in the comments above, there might be a lot more fun when I will try to execute this.

Why are you using these extra packages? In the following script, I do not see anything that would not be covered by the testapi package.

Can you point out if there are any mandatory packages that I will need to import... I used the podman.pm as base. Podman had all the packages and I assumed that if podman would need them, toolbox may need them as well.

When you take a look at the podman.pm test, you will see that there are subroutines (methods) that are not provided by the testapi, such as mutex_create. Also, the podman test seems to be working with a podman server that needs to be created before you want to work with that. Therefore you need to use those special modules to allow this. In your tests, you are not using anything like that, therefore it is not necessary to load them.

@lruzicka I have fixed mostly taken into account all your suggestions and made changes. I am still waiting on your review and the packages that must be imported to make the test case executable.
I have fixed the execution lineup and now this should be pretty doable. Can we run this somewhere and see what is the actual outcome?
If I recall what @adamw mentioned in the comments above, there might be a lot more fun when I will try to execute this.

I will take a look on the new version soon.

You have decided to remove the only required library testapi and let the unnecessary rest stay. Let me elaborate more:

1. use base "installedtest"; tells that this test inherits from installed tests, which is mostly fine for post-install tests.
2. use strict; should always be here, because you do not want Perl to give up on script checking. Leaving this out could have interesting consequences of how your namespaces will be treated, etc.
3. use warnings; can come handy, because this will make sure the warning messages are displayed. openQA might have this option switched on automatically, I am not sure, but putting it here costs you nothing and it makes sure you get those warnings.
4. lockapi, mmapi, utils, and tapnet are not required, because you are not using any subroutines except the standard ones.
5. However, to have access to the standard routines, you need to make use testapi;.

Why do you install the toolbox and then check that it is installed on line 15? If you have installed it and the command has run fine (you are checking for 0 exit so that should be fine) they you know for sure that toolbox is installed and you do not need to check for it.

Basically, you have two options here:

1. You will always attempt to install toolbox. If it is already installed, DNF will not reinstall it and will quit. The exit code will also be 0 in this case, so you are safe to go. However, you installing the toolbox will cost you some time.
2. You will check if toolbox is installed and only install it, when it is not.

Note, that it depends on the actual ISO image, because some isos will have toolbox installed and some won't. If you know for sure, that the image will not have it installed, then proceed with number 1.

I think you wanted to use validate_script_output here.

You cannot run this command from within the container1 as podman is not part of that container. You need to exit the container first.

This command requires human interaction. You have left out the -y option.

This runs ok, however does it make sense to run it from within the container when you could only do cat /etc/redhat-release on the command line and you'd get the same result? Wouldn't it be more logical to exit the container first and then run that command? Or best of all. Should this command be run when the container is stopped, so that we know it will be started and the command correctly executed?

I do not know really, but it seems more logical to me.

When you enter the container, you only check that the enter command has returned a 0. Maybe you should also consider to check that the container has been correctly entered, and if exited that it was properly exited. Maybe you could use the uname -a command to run from within the container to confirm it says "Linux toolbox" and when entered and something else when exited. Or you could utilize a needle (although I would stick to a needless variant) and check for that fancy prompt with the violet dot.

Also, your code is failing because of some Perl issues. Please, check the Zuul outcome and try to investigate why. If you need help, let me know.

@adamw, I have made the changes, can we now attempt to run it ? (after making changes to the fiff template which I am sure I will have fun learning)

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/088e641eedef49e79cae1571a8e796fd

• os-autoinst-tox : SUCCESS in 5m 06s
• os-autoinst-perl : FAILURE in 7m 27s

The perl test indicates there's still an issue: it's validate_script_output, not validate_script_run.

I'll look at showing you what to add to templates for this tomorrow.

The perl test indicates there's still an issue: it's validate_script_output, not validate_script_run.

This is done, I just pushed the change. Thanks for pointing it out :)
Also, thank you again!

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/c5c897b6168048f687a6f5fd13959c16

• os-autoinst-tox : SUCCESS in 5m 17s
• os-autoinst-perl : SUCCESS in 8m 08s

@sumantrom, you still have not commented on or incorporated my remarks into the code. I still believe, they might help make it leaner (at least).

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/eabb8676aee24637a6fde024a2b22e40

• os-autoinst-tox : SUCCESS in 5m 28s
• os-autoinst-perl : SUCCESS in 7m 16s

@sumantrom, you still have not commented on or incorporated my remarks into the code. I still believe, they might help make it leaner (at least).

I just removed the unnecessary stuff and ran tidy again. Things look good for now. Thanks again Lukas

Copied from an email I sent sumantro earlier:

What you want to do more or less is look at existing entries in templates.fif.json and templates-updates.fif.json . They're very long, but don't get overwhelmed. You should only need to touch the TestSuites section; hopefully you can kinda see what's going on there: it's a dict-of-dicts, the keys are the "test suite names" (what you might usually think of as the "test names"). The "profiles" define what flavors (flavors map to images for compose tests, mostly, and something like 'environments' for update test) the test suites run in - the numbers indicate the test's priority, lower is higher priority, don't worry about them too much. The 'settings' are openQA settings vars that are specific to this test suite.

You can look at existing fairly simple "boot an installed system then test this app" cases to get an idea what your new entry/entries should look like, e.g. desktop_terminal:

    "desktop_terminal": {
        "profiles": {
            "fedora-KDE-live-iso-x86_64-*-64bit": 22,
            "fedora-Silverblue-dvd_ostree-iso-ppc64le-*-ppc64le": 50,
            "fedora-Silverblue-dvd_ostree-iso-x86_64-*-64bit": 50,
            "fedora-Workstation-live-iso-ppc64le-*-ppc64le": 20,
            "fedora-Workstation-live-iso-x86_64-*-64bit": 20,
            "fedora-Workstation-upgrade-x86_64-*-64bit": 40,
            "fedora-Workstation-upgrade-aarch64-*-aarch64": 40,
            "fedora-Workstation-raw_xz-raw.xz-aarch64-*-aarch64": 22
        },
        "settings": {
            "BOOTFROM": "c",
            "HDD_1": "disk_%FLAVOR%_%MACHINE%.qcow2",
            "POSTINSTALL": "desktop_terminal",
            "START_AFTER_TEST": "%DEPLOY_UPLOAD_TEST%"
        }
    },

you might want your test to run in a different set of environments, which you can copy from other entries - you don't need to worry about exactly what the profiles mean right now (but they're defined higher up in the file if you're curious, basically a combination of a flavor and a 'machine'). Note, any time you see %SOMETHING%, that's a substitution - the scheduler will replace that string with the value of the variable SOMETHING, after resolving the whole variable resolution order (which is a bit complex).

The 'settings' for your test should be pretty similar:

• BOOTFROM c means boot from the hard disk (not any attached ISO)
• POSTINSTALL desktop_terminal means that after booting an installed system in the standard way openQA should run that test module (so change it to the name of yours)
• START_AFTER_TEST %DEPLOY_UPLOAD_TEST% means, more or less, "for each profile, run after the test that does a standard install then uploads a disk image". This is usually install_default_upload , but for a few flavors it's something else, so we use this indirection via another test setting (which is defined elsewhere for each flavor).
• HDD_1 is the disk image to use for the first (and in this case only) hard disk. This value is always correct for tests that use START_AFTER_TEST %DEPLOY_UPLOAD_TEST%, because all the DEPLOY_UPLOAD_TESTs are written to upload their disk image under that name. So it's kinda boilerplate.

You can add an entry to templates-updates.fif.json to run the test on updates too, but maybe it's best to just focus on composes first.

Oh, and I think we try to keep the list in alphabetical order by test suite name as best we can...

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/0cbd182f14c0483ba78012a8d44521fc

• os-autoinst-tox : FAILURE in 4m 59s
• os-autoinst-perl : SUCCESS in 7m 37s

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/9a5cfee3563e456bba2611b693dbc42e

• os-autoinst-tox : SUCCESS in 5m 21s
• os-autoinst-perl : SUCCESS in 7m 09s

The templates stuff looks mostly good, thanks. Just a couple of things: you're missing spaces between the : and { on several lines (this isn't technically invalid, it's just a style thing, we always use spaces in this file), and this probably doesn't need to run on both BIOS and UEFI. Actually, it probably can't, because we don't have separate DEPLOY_UPLOAD_TESTs for BIOS and UEFI - it needs to run on the same machine as the DEPLOY_UPLOAD_TEST runs on, which in this case is the BIOS one (64bit). Just drop the UEFI line. OTOH, it seems like it might be a good idea to run this on Silverblue as well? That seems like one of the primary places where it's actually used in real life. Might want to add a line for fedora-Silverblue-dvd_ostree-iso-x86_64-*-64bit.

Oh, and maybe drop the priorities to about 40.

Thanks!

Still, it's good enough to test with, so https://openqa.stg.fedoraproject.org/tests/3060973 should be the first attempt to run this for real - let's see what happens.

OK, so kinda as we suspected, this fails:

https://openqa.stg.fedoraproject.org/tests/3060973#step/toolbox/13

it fails because the toolbox enter command doesn't return while the toolbox is open, so we can't use assert_script_run.

What I'd suggest in this case is to instead do type_string 'toolbox enter container1\n'; and then assert_screen "console_in_toolbox", 180; - if you look in lib/utils.pm, we do basically this there, in _repo_setup_updates, when we need to use a toolbox container. That needle just matches on the little purple blob that indicates we're in a container.

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/b8925561be734d2682c391a7096208b4

• os-autoinst-tox : SUCCESS in 4m 31s
• os-autoinst-perl : SUCCESS in 7m 15s

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/ed3d6af362f34554a58332f5b0bc03de

• os-autoinst-tox : SUCCESS in 5m 41s
• os-autoinst-perl : SUCCESS in 7m 40s

next test run: https://openqa.stg.fedoraproject.org/tests/3067889

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/d284b3d8ef3e4ead9ac6dd06d2d667ff

• os-autoinst-tox : SUCCESS in 7m 19s
• os-autoinst-perl : SUCCESS in 8m 45s

OK, so it failed again. The reason is you didn't put a \n at the end of type_string "exit"; , so it actually winds up typing "exitpodman stop container1", which obviously isn't gonna work. :D

Another thing I noticed while watching it run: validate_script_output "toolbox list | grep container1", sub { m/container1/ }; is kinda redundant. You're already grepping the output for "container1". Just assert_script_run "toolbox list | grep container1" would be fine: grep will return non-0 if the output doesn't contain "container1".

OK, so it failed again. The reason is you didn't put a \n at the end of type_string "exit"; , so it actually winds up typing "exitpodman stop container1", which obviously isn't gonna work. :D

Another thing I noticed while watching it run: validate_script_output "toolbox list | grep container1", sub { m/container1/ }; is kinda redundant. You're already grepping the output for "container1". Just assert_script_run "toolbox list | grep container1" would be fine: grep will return non-0 if the output doesn't contain "container1".

Thanks for the review, I just fixed it.

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/e4f10e2cc457402dbb26fdd08737be88

• os-autoinst-tox : SUCCESS in 5m 22s
• os-autoinst-perl : SUCCESS in 6m 31s

@adamw , is it passing now?

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/b016295f37d94f1089f4fda51f98d723

• os-autoinst-tox : SUCCESS in 5m 04s
• os-autoinst-perl : SUCCESS in 6m 55s

sorry, last few days were a bit crazy. will try it now. edit: https://openqa.stg.fedoraproject.org/tests/3078727

OK, so it passes now, but I had to make several changes:

--- a/tests/toolbox.pm
+++ b/tests/toolbox.pm
@@ -20,6 +20,8 @@ sub run {
     type_string "toolbox enter container1\n";
     assert_screen "console_in_toolbox", 180;
     type_string "exit\n";
+    sleep 3;
+    assert_script_run "clear";
     # Stop a conatiner
     assert_script_run 'podman stop container1';
     # Toolbox remove container
@@ -27,13 +29,14 @@ sub run {
     # Toolbox remove image and their dependant containers
     assert_script_run "toolbox rmi --all --force";
     # create a rhel image with distro and release flags
-    assert_script_run "toolbox create --distro rhel --release 9.1", 300;
+    assert_script_run "toolbox -y create --distro rhel --release 9.1", 300;
     # validate rhel release file to ensure correct version
     type_string "toolbox enter rhel-toolbox-9.1\n";
     assert_screen "console_in_toolbox", 180;
     type_string "exit\n";
+    sleep 3;
     #run a specific command on a given choice of distro and release
-    validate_script_output "toolbox run --distro rhel --release 9.1 cat /etc/redhat-release", sub { m/Red Hat Enterprise Linux release 9.1 (Plow)/ };
+    validate_script_output "toolbox run --distro rhel --release 9.1 cat /etc/redhat-release", sub { m/Red Hat Enterprise Linux release 9.1 \(Plow\)/ };

• we need to sleep for a bit after typing exit to give the exit a chance to actually happen, or else the next keystrokes get swallowed by the dying container. Just sleeping is a bit lazy - we could maybe do wait_still_screen instead, but be careful of flashing cursors (drop the expected similarity to 38)
• we need to clear between containers, or else the assert_screen "console_in_toolbox" matches immediately on the "in container" marker from the previous container, and we start typing a command intended for the container before we're actually in it
• missing -y for the RHEL toolbox creation
• in the final validate_script_output, we need to escape the parentheses in the expected output, because they are special characters in regex (they indicate a character group)

I'll let you make the changes. There's also a couple of typos in comments - "container" spelt wrong in one line, and "dependent" spelt wrong in another.

Note I rebased the PR, so you'll need to reset --hard your local checkout.

Note I rebased the PR, so you'll need to reset --hard your local checkout.

Thanks a lot @adamwill I have fixed the code with all the things you mentioned!!

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/45764e6d17e142ffb38c7c669fa7bb85

• os-autoinst-tox : SUCCESS in 5m 31s
• os-autoinst-perl : FAILURE in 7m 42s

You got the escaping wrong. It should be \(Plow\), not \(\Plow) - you want a blackslash before each parenthesis.

You got the escaping wrong. It should be \(Plow\), not \(\Plow) - you want a blackslash before each parenthesis.

Ahh, its escape room all over again :D .. I fixed it

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/ae86411cf06e443bbfa95b9fbfafe3aa

• os-autoinst-tox : SUCCESS in 4m 42s
• os-autoinst-perl : FAILURE in 6m 40s

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/49ea5fd446564bdc8fc2f9669b14a2ae

• os-autoinst-tox : SUCCESS in 5m 03s
• os-autoinst-perl : FAILURE in 6m 25s

Looks like it needs tidying again. That might be my fault, I didn't check my changes with tidy :D

Looks like it needs tidying again. That might be my fault, I didn't check my changes with tidy :D
Running and pushing as we speak

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/7b77ae114726486784b0e2d89a78d25d

• os-autoinst-tox : SUCCESS in 4m 25s
• os-autoinst-perl : FAILURE in 6m 57s

hum, it seems like the check isn't happy about utils.pm, which you didn't touch. but tidy on my laptop thinks it's ok. i'll look into this and resolve it somehow in the morning, gotta sleep now.

could you rebase on current main and squash all your commits down to one? first do git rebase main then git rebase -i main and change the command for all commits except the first to be 'squash', that should do it.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/eeded253f3fc4db7b8f6e06d3cbf5ff1

• os-autoinst-tox : SUCCESS in 5m 16s
• os-autoinst-perl : FAILURE in 7m 07s

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/9aa779103b134dec8d06023637c0883d

• os-autoinst-tox : SUCCESS in 5m 20s
• os-autoinst-perl : FAILURE in 6m 38s

well, that didn't work. :D it seems like now you somehow have the flattened commit on top of the others.

if you now do git rebase -i main again and drop all commits except the first one ( a2d39e4fd41e2c32390f9bd8409afe833c441cd0 ), I think it should work.

as a general rule for the future, commits should be "logical units". If you're working on a commit to do X, it goes to review, and we decide to make a change to it, but it's still just doing X, you should just make the change and then amend the commit and force push it. Don't add another commit on top of it; we don't need the history of the revision of the pull request in the repository, once the PR is merged. a PR should only have multiple commits if it makes sense to break up the work involved in the PR into multiple 'units', even once the PR is merged (sometimes this is the case).

you obviously should never amend a commit that's already made it to the main branch, or force push to the main branch (or any branch that isn't a PR branch). but amending and force-pushing PR branch commits is totally fair game.

oh, also, you should edit the commit message of the remaining commit to be just "add toolbox test case", optionally with some additional descriptive text if you like. again, we don't need the history of the PR in the commit message.

huh, I just found the thing tidy is complaining about (I think), no idea how that isn't causing more problems, but anyway, I fixed it.

Can you try and do this?

• git checkout main
• git pull
• git checkout toolbx-test
• git rebase -i main
• set all commits except a2d39e4fd41e2c32390f9bd8409afe833c441cd0 to 'drop' or just 'd'
• git commit --amend
• adjust the commit message

then, if you do git log and git show, you should see just a single commit on top of main, with your changes in it, with a sensible description. if that's the case, we should be good! you can push it. if not, uh, yell. :) (and don't push it in case we lost the changes or something).

huh, I just found the thing tidy is complaining about (I think), no idea how that isn't causing more problems, but anyway, I fixed it.

Can you try and do this?

• git checkout main
• git pull
• git checkout toolbx-test
• git rebase -i main
• set all commits except a2d39e4fd41e2c32390f9bd8409afe833c441cd0 to 'drop' or just 'd'
• git commit --amend
• adjust the commit message

then, if you do git log and git show, you should see just a single commit on top of main, with your changes in it, with a sensible description. if that's the case, we should be good! you can push it. if not, uh, yell. :) (and don't push it in case we lost the changes or something).

I can't find a2d39e4fd41e2c32390f9bd8409afe833c441cd0. I do find a looong list and here's the last part of it.

#!git
pick f1a6c91 Drop a couple of webUI conditionals to 39, not 40`
pick f22af41 ostree: avoid rebasing from F39+ to <F39
pick a3a8861b add toolbox test case
pick cdd75998 add toolbox test case
pick 5413c1d5 fixing something with tidy
pick 8db8f9bb reduce wait time
pick 124e7956 change def timeouts, exceution process
pick 792564a4 ran tidy
pick 9a3dd136 minor revision
pick 03c19726 fix syntax
pick 5c3e203e remove unncessary apis
pick 00fa844c add toolbox to templates.fif
pick d5e1a245 needles to look for purple blob
pick 8246d07b add changes
pick 387d4ba8 add template
pick e31c8105 add delimiter
pick f14f8a1a noticed another faultline with rhel container
pick fd887a6f add delimiters to type_string
pick efd92a6b final code
pick 60b2be79 fix escaping
pick ea16abc1 might fix tidy studd and add extra comments
pick 85347f03 ran tidy again

When I do pick a2d39e4f in here ; it still doesn't grab the commit link.. like the others does... am I missing something?

Ah, I think when I look at your PR locally I get different commit IDs. The commit we want to keep is a3a8861b25f6369143aaacc2727a021510f89aba . But I don't know why it's picking a couple of commits from main on top of your commits there, that seems weird.

What we want is to wind up with just a3a8861b25f6369143aaacc2727a021510f89aba on top of current main.

Hum. Actually, I'm basing this on how I set up my checkouts. Maybe your setup is different - if you don't have your main branch tracking the 'official' repo's main branch, that could be an issue. Can you post your .git/config file from your checkout, please? That way I can see how you have your remotes set up.

Once we get this sorted out I can give you a quick guide to how I set up my checkouts and do pull requests, maybe :)

Ah, I think when I look at your PR locally I get different commit IDs. The commit we want to keep is a3a8861b25f6369143aaacc2727a021510f89aba . But I don't know why it's picking a couple of commits from main on top of your commits there, that seems weird.

What we want is to wind up with just a3a8861b25f6369143aaacc2727a021510f89aba on top of current main.

Hum. Actually, I'm basing this on how I set up my checkouts. Maybe your setup is different - if you don't have your main branch tracking the 'official' repo's main branch, that could be an issue. Can you post your .git/config file from your checkout, please? That way I can see how you have your remotes set up.

Once we get this sorted out I can give you a quick guide to how I set up my checkouts and do pull requests, maybe :)

This is my config

[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
[remote "origin"]
        url = ssh://git@pagure.io/forks/sumantrom/fedora-qa/os-autoinst-distri-fedora.git
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "main"]
        remote = origin
        merge = refs/heads/main
[branch "toolbx-test"]
        remote = origin
        merge = refs/heads/toolbx-test
[pull]
        rebase = true 

I really need to know how all of this is set up on your end .. that might make life sooo much easy..

My workflow usually is

Fork repo to my own user so sumantrom/os-auto....
Clone this "forked" repo
Create a checkout branch
Add commits to this checkout branch
Before pushing any code , run a git pull to get latest changes from origin/main formerly master
push change and raise a PR to main
Post that git automagically tracks the PR and posts commit to the PR itself unless I try to merge it
I have never actually had any problem merging and squashing before, it can however be something... :\

So, this is how mine looks:

[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
[remote "origin"]
        url = ssh://git@pagure.io/fedora-qa/os-autoinst-distri-fedora.git
        fetch = +refs/heads/*:refs/remotes/origin/*
[remote "fork"]
        url = ssh://git@pagure.io/forks/adamwill/fedora-qa/os-autoinst-distri-fedora.git
        fetch = +refs/heads/*:refs/remotes/fork/*
[branch "main"]
        remote = origin
        rebase = true
        merge = refs/heads/main

for most repos, there would also be PR branches that have remote = fork - for this repo I don't really have any, because I tend to just push my PR branches to origin so I can test them easily on lab. Of course, I can do that with this project because I'm one of the owners :) I don't think you can.

The key is that I set the remote called origin, which is kinda the "default", to be the 'original' repo - pagure.io/fedora-qa/os-autoinst-distri-fedora . I then have a second remote, called fork, which is my fork of the repo, where PRs would usually be pushed to - pagure.io/forks/adamwill/fedora-qa/os-autoinst-distri-fedora .

Also note that the main branch in my checkout tracks the remote called origin, not the remote called fork. The benefit of this is that when I do git pull on main, I get the latest state of the 'original' repo's main branch, not main from my fork, which means I don't get stuck with a stale main, or constantly have to resync my fork's main branch with the original repo's.

So when I do git checkout main, git pull, then git checkout someprbranch and git rebase main, I'm rebasing the PR branch on the current state of main from the 'original' repo.

With your setup, your main branch is tracking the main branch of your fork - https://pagure.io/fork/sumantrom/fedora-qa/os-autoinst-distri-fedora/commits/main - which is two months behind the original repo's main branch. This is probably why rebasing on main isn't giving you the results I'm hoping for.

With my setup, I would do this to try and clean up the PR:

git checkout main
git pull (this would get my local checkout's main branch even with the original repo's)
git checkout toolbx-test
git reset --hard main (this would make the PR branch identical with main, temporarily)
git cherry-pick a3a8861 (this would take just that commit on top of current main, which is the state we want; I'd then verify that's really what we have, with git log and git show)
git commit --amend to rewrite the commit message
git push --force to update the PR

yaay @adamwill.. it was the config file .. I wish we know ..pheww :D Thanks for the help. I will set up my configs this way from here on

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/0ba59593b7da46f094e2ea48f289bb9d

• os-autoinst-tox : SUCCESS in 5m 08s
• os-autoinst-perl : SUCCESS in 6m 51s

awesome! that looks better. i'll deploy it on stg overnight for a final check then merge it in the morning. thanks!

OK, this looks to be working well on lab.