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

Also remove https repos in zypper_clear_repos #10661

Merged
merged 1 commit into from Jul 9, 2020

Conversation

DrMullings
Copy link
Contributor

This time with a regex in the original find expression

Followup to #10578
Verification needed

Copy link
Member

@Vogtinator Vogtinator left a comment

Choose a reason for hiding this comment

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

Was too quick - wrong type of grouping. This would also match hththptphhths://foo.

@@ -26,7 +26,7 @@ sub run {
my $repos_folder = '/etc/zypp/repos.d';
zypper_call 'lr -d', exitcode => [0, 6];
assert_script_run(
"find $repos_folder/*.repo -type f -exec grep -q 'baseurl=http://download.opensuse.org/' {} \\; -delete && echo 'unneed_repos_removed' > /dev/$serialdev",
"find $repos_folder/*.repo -type f -exec grep -Eq 'baseurl='[http|https]://download.opensuse.org/' {} \\; -delete && echo 'unneed_repos_removed' > /dev/$serialdev",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"find $repos_folder/*.repo -type f -exec grep -Eq 'baseurl='[http|https]://download.opensuse.org/' {} \\; -delete && echo 'unneed_repos_removed' > /dev/$serialdev",
"find $repos_folder/*.repo -type f -exec grep -Eq 'baseurl='(http|https)://download.opensuse.org/' {} \\; -delete && echo 'unneed_repos_removed' > /dev/$serialdev",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adapted

@Vogtinator
Copy link
Member

Thanks for merging, but maybe we should've verified it first?

It's not merged yet - without a verification run and success from travis that wouldn't be a good idea. The approval rating is only for code review AFAIK.

@DrMullings
Copy link
Contributor Author

Confused the merged badge from the old one, sorry that

@@ -26,7 +26,7 @@ sub run {
my $repos_folder = '/etc/zypp/repos.d';
zypper_call 'lr -d', exitcode => [0, 6];
assert_script_run(
"find $repos_folder/*.repo -type f -exec grep -q 'baseurl=http://download.opensuse.org/' {} \\; -delete && echo 'unneed_repos_removed' > /dev/$serialdev",
"find $repos_folder/*.repo -type f -exec grep -Eq 'baseurl='(http|https)://download.opensuse.org/' {} \\; -delete && echo 'unneed_repos_removed' > /dev/$serialdev",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"find $repos_folder/*.repo -type f -exec grep -Eq 'baseurl='(http|https)://download.opensuse.org/' {} \\; -delete && echo 'unneed_repos_removed' > /dev/$serialdev",
"find $repos_folder/*.repo -type f -exec grep -Eq 'baseurl=(http|https)://download.opensuse.org/' {} \\; -delete && echo 'unneed_repos_removed' > /dev/$serialdev",

Copy link
Member

Choose a reason for hiding this comment

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

+1 was just gonna comment the same

This time with a regex in the original find expression
@DrMullings
Copy link
Contributor Author

https://openqa.opensuse.org/tests/1327407

https://openqa.opensuse.org/tests/1327408

Removed the Quote and started one NET and one DVD verification

Copy link
Member

@Vogtinator Vogtinator left a comment

Choose a reason for hiding this comment

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

Still works locally, though now without hypothetical false positives.

Maybe matching 'baseurl=[^:]+://download.opensuse.org/' is a better option, though currently we mostly use HTTP(s) everywhere.

@DrMullings
Copy link
Contributor Author

Still works locally, though now without hypothetical false positives.

Maybe matching 'baseurl=[^:]+://download.opensuse.org/' is a better option, though currently we mostly use HTTP(s)

Wouldn't this match everything before ':' ? I would prefer keeping it as an OR, but I am no regex expert

My openqa-clone-custom-git-refspec seems still broken though, so if someone could run

openqa-clone-custom-git-refspec https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/10661 https://openqa.opensuse.org/tests/1327002 TEST=jrauch_test GROUP=0 _SKIP_POST_FAIL_HOOKS=1
openqa-clone-custom-git-refspec https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/10661 https://openqa.opensuse.org/tests/1325341 TEST=jrauch_test GROUP=0 _SKIP_POST_FAIL_HOOKS=1

we can verify it on O3

@Vogtinator
Copy link
Member

Done:

https://openqa.opensuse.org/t1327410
https://openqa.opensuse.org/t1327411

@Vogtinator
Copy link
Member

Still works locally, though now without hypothetical false positives.
Maybe matching 'baseurl=[^:]+://download.opensuse.org/' is a better option, though currently we mostly use HTTP(s)

Wouldn't this match everything before ':' ? I would prefer keeping it as an OR, but I am no regex expert

Yes, so also rsync://, ftps://, htTpS:// or just baseurl="http://.

@DrMullings
Copy link
Contributor Author

Both verification are fine, travis has no complaints, can we merge this or do you insist on matching different protocols other than http(s)?

@Vogtinator
Copy link
Member

Both verification are fine, travis has no complaints, can we merge this or do you insist on matching different protocols other than http(s)?

I'm fine with either.

@DrMullings
Copy link
Contributor Author

Then merge it please

@Vogtinator Vogtinator merged commit 0c5d8fa into os-autoinst:master Jul 9, 2020
XGWang0 pushed a commit to XGWang0/os-autoinst-distri-opensuse that referenced this pull request Jul 9, 2020
This time with a regex in the original find expression
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants