Skip to content

Disable git_clone minion jobs #237

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

perlpunk
Copy link
Contributor

We are using fetchneedles with --depth 1 for the initial clone. We don't need to fetch the whole osado repo in this test.

As a result of this we can also reduce the retry attempts and timeout again.

After #236

@okurz
Copy link
Member

okurz commented Apr 11, 2025

We are using fetchneedles with --depth 1 for the initial clone. We don't need to fetch the whole osado repo in this test.

I agree that we don't need it but we only run the "full osado" test in lower intervals. Wouldn't it be a good test for real behaviour to keep the git_clone minion jobs? Or as alternative why do we even still call fetchneedles? I guess https://progress.opensuse.org/issues/164898 at least from the title point of view was not yet completed?

I created https://progress.opensuse.org/issues/180854 "Deprecate fetchneedles as we now have git_clone minion jobs" for that

@perlpunk
Copy link
Contributor Author

Or as alternative why do we even still call fetchneedles? I guess https://progress.opensuse.org/issues/164898 at least from the title point of view was not yet completed?

From https://progress.opensuse.org/issues/164898

Out of scope
Doing any kind of initial checkout if git working copies do not exist yet

@okurz
Copy link
Member

okurz commented Apr 11, 2025

Or as alternative why do we even still call fetchneedles? I guess https://progress.opensuse.org/issues/164898 at least from the title point of view was not yet completed?

From https://progress.opensuse.org/issues/164898

Out of scope
Doing any kind of initial checkout if git working copies do not exist yet

true true. wasn't that done nevertheless as part of another ticket then?

@perlpunk
Copy link
Contributor Author

From https://progress.opensuse.org/issues/164898

Out of scope
Doing any kind of initial checkout if git working copies do not exist yet

true true. wasn't that done nevertheless as part of another ticket then?

Nope. At least not for jobs that don't have CASEDIR/NEEDLES_DIR

@perlpunk perlpunk force-pushed the disable-minion-git-clone branch from 6bbbaba to bad0bce Compare April 11, 2025 10:10
We are using fetchneedles with --depth 1 for the initial clone.
We don't need to fetch the whole osado repo in this test.

As a result of this we can also reduce the retry attempts and timeout again.
@perlpunk perlpunk force-pushed the disable-minion-git-clone branch from bad0bce to 304e596 Compare April 11, 2025 10:11
@perlpunk perlpunk marked this pull request as ready for review April 11, 2025 10:11
@perlpunk
Copy link
Contributor Author

Wouldn't it be a good test for real behaviour to keep the git_clone minion jobs?

openqa_install+publish and openqa_from_bootstrap are still using that for running a distri-example test

@@ -35,6 +35,10 @@ sub install_from_pkgs {
assert_script_run 'systemctl enable apache2';
assert_script_run 'systemctl restart apache2';
}
if (get_var('FULL_OPENSUSE_TEST')) {
# avoid second git fetch after fetchneedles
assert_script_run q{if [ -e /etc/openqa/openqa.ini ]; then sed -i -e 's/#\[scm git\]/[scm git]\ngit_auto_clone = no\ngit_auto_update = no/' /etc/openqa/openqa.ini; else echo -e "[scm git]\ngit_auto_clone = no\ngit_auto_update = no" > /etc/openqa/openqa.ini.d/git.ini; fi};
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can just assume that /etc/openqa/openqa.ini.d is supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this in the install_from_pkgs. I just wonder if this is the best place to inject this. Following the logic from run, I wouldnt expect this hidden here maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this in the install_from_pkgs.

That's the only place where we edit openqa.ini currently. Directly below this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can just assume that /etc/openqa/openqa.ini.d is supported.

I just used the same as below, so I'd say it would just look weird if we still have that logic below but not in the code I introduce.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dont want to be destructive but I think it make sense to take those openqa.ini out of that function, wrap them into another with a corresponding name and add it in the run function. it will look more clear and will keep the functions clean. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why you're all complaining about this line but did not complain about line 43 below

Copy link
Contributor

Choose a reason for hiding this comment

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

Because line 43 was introduced when the new config parsing was just introduced so it had to support the old and new behavior. However, I don't think this is required anymore so always creating a drop-in config file should be enough.

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.

4 participants