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

TESTS: Re-add tests for `kdestroy -A` #546

Closed
wants to merge 1 commit into from

Conversation

@fidencio
Copy link
Contributor

commented Mar 29, 2018

This reverts commit 89726be and also do
a few modifications on it in order to ensure we don't have any
regression on https://pagure.io/SSSD/sssd/issue/3413

As this patch depends on a krb5 patch applied to the distros we run our internal CI on, I've opened a bug report providing patches for Fedora0 and Debian1.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

I'll ack and push the patch if you show me a CI run from our internal Jenkins :-)

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

but the hunk itself of course LGTM

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

so, not totally related to this thread, but is this one of the cases where a label like "blocked" or "depends-on" would be useful? iirc you suggested something like this on sssd-devel the other day.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2018

Yep, that's exactly the situation I could see the "blocked"/"depends-on" tag being used.

@fidencio fidencio added the Blocked label Mar 29, 2018
@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

On my up-to-date F-27 this test already passes with krb5-libs-1.15.2-8.fc27.x86_64. I started CI but I don't have the results yet.

The only small issue is how to make sure that on other platforms where the test might fail, it is at least easy to see why it fails. So I would propose to either move the test with 3 principals to a separate function and add a doc comment or at least add a better comment that this might fail due to a krb5 bug.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

@jhrozek, The problem here is that we're missing:

IMHO, there's not much we can do than wait till the debian patch gets merged, unfortunately.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2018

We can mark the test as expected failure. I'm not sure myself if it's better or not, but tl;dr marking the test as expected failure would still print "failed" when you run the tests from the command line, but not fail the whole testsuite.

Personally, I would be OK with that because I run the test on Fedora where the bug is fixed.

@alexey-tikhonov

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

retest this please

@alexey-tikhonov

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

Hi @fidencio,
is there any chance you could rebase this on the top of current master, please?

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

@alexey-tikhonov,

Thanks for taking a look in the PR. At this point, being out of SSSD project for more than 1 year, without touching this PR for more than 1.5 years, I don't actually feel comfortable rebasing it and re-submitting without giving it some proper tests. Unfortunately, I have no time for testing it properly.

In the end, it's a 10 lines patch, feel free to just copy and paste it accordingly. Mentioning the original author (me) is highly desirable.

Best Regards,

This reverts commit 89726be and also do
a few modifications on it in order to ensure we don't have any
regression on https://pagure.io/SSSD/sssd/issue/3413

Related:
https://pagure.io/SSSD/sssd/issue/3413

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
@alexey-tikhonov alexey-tikhonov force-pushed the fidencio:wip/kdestroy-A branch from 2aa613a to 626aff7 Sep 18, 2019
@alexey-tikhonov

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

retest this please

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Alexey, I don't know if Debian already picked up the fixed libkrb5. If not, I think it would be nice to add this as a separate test and mark it as xfail or similar.

@alexey-tikhonov

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Alexey, I don't know if Debian already picked up the fixed libkrb5.

I guess yes as GH CI: "sssd-ci/debian10 — Success."

But I will check.

@alexey-tikhonov

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

Debian10 has fixed libkrb5. Fedora picked up fix long time ago. And even CentOS CI seems to be fine.

ACK.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

  • master
    • 43aae7e - TESTS: Re-add tests for kdestroy -A
@pbrezina pbrezina closed this Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.