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

Fix proxy-auth and proxy-kickstart tests #1184

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

jikortus
Copy link
Contributor

This covers issue #1108.

The change of dnf command/options order is necessary to work around https://bugzilla.redhat.com/show_bug.cgi?id=2280877 (DNF5 seems to have a bit of inconsistency in how it handles the options order, at least in some cases, compared to DNF4).

proxy-kickstart.sh Fixed Show fixed Hide fixed
@jikortus
Copy link
Contributor Author

@rvykydal FYI I hope the Travis CI failure is not caused by the tests - they pass just fine for me locally (executed via ./containers/runner/launch --platform fedora_rawhide --run-args "--security-opt label=disable" proxy-auth).

@rvykydal
Copy link
Contributor

/test-os-variants

@rvykydal
Copy link
Contributor

Thank you for the fix!
Please add also re-enabling (in skip-testtypes) of the test into the PR (similar as for example in the #1193) so that it can be tested here with /test-os-variants comment.
I think the PR would perhaps resolve also #680 for fedora? If so, please remove also this disablement.

proxy-auth.ks.in Outdated
os_version=$(. /etc/os-release && echo ${VERSION_ID%%.*})
platform=$(. /etc/os-release && echo ${ID})

if [ "${platform}" == "rhel" ] && [ ${os_version} -lt 10 ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the per-platform conditions, there is an open PR #1174 which adds get_platform() function that could be used similarly as in repo-install test when the PR is merged. I hope we merge it soon so you can use it. Or I can take care of it in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The #1174 is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! In the end I realized I can actually use the substituted values of OS name and version (I'd have to have them separated in the condition expressions anyway), so I used this approach instead of get_platform() - I hope it's OK.

@jikortus
Copy link
Contributor Author

jikortus commented Jun 3, 2024

@rvykydal Thanks for the review! Yes, I believe this PR should also fix issue #680, as the tests pass just fine on Rawhide.

@jikortus
Copy link
Contributor Author

jikortus commented Jun 5, 2024

/test-os-variants

1 similar comment
@rvykydal
Copy link
Contributor

/test-os-variants

proxy-auth.sh Outdated
@@ -29,6 +29,9 @@ prepare() {
local tmp_dir=$2
local httpd_url=""
local proxy_url=""
httpd_url=$(cat ${tmpdir}/httpd_url)
proxy_url=$(cat ${tmpdir}/proxy_url)
local proxy_rc
Copy link
Contributor

Choose a reason for hiding this comment

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

I think proxy_rc is not used and we can remove it.

@@ -30,6 +30,9 @@ prepare() {
local tmp_dir=$2
local httpd_url=""
local proxy_url=""
httpd_url=$(cat ${tmpdir}/httpd_url)
proxy_url=$(cat ${tmpdir}/proxy_url)
local proxy_rc
Copy link
Contributor

Choose a reason for hiding this comment

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

I think proxy_rc is not used and we can remove it.

Copy link
Contributor

@rvykydal rvykydal 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 to me, thank you!

@jikortus
Copy link
Contributor Author

/test-os-variants

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.

2 participants