CI: Use noble image for QA tests #641
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #641 +/- ##
==========================================
- Coverage 83.28% 83.26% -0.02%
==========================================
Files 80 80
Lines 8617 8607 -10
Branches 75 74 -1
==========================================
- Hits 7177 7167 -10
Misses 1112 1112
Partials 328 328 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
3v1n0
force-pushed
the
ci-noble
branch
2 times, most recently
from
c579afa
to
671525e
Compare
Some tests golden files were not including the whole results, let's fix them by increasing the terminal heights.
In this way we can check for them being in golden files in a reliable way
Add an helper function to check that the golden files contains the final pam runner results strings. In this way we can ensure that the golden files sizes are big enough to hold all the terminal contents.
That's the ubuntu version we're targetting so far, and so we should care about having it working as expected when doing integration tests in particular.
When running as root or in a schroot we should not care about chrome sandbox used by VHS, so let's ignore it.
These are not needed anymore as the libPAM leaks are fixed in noble
3v1n0
force-pushed
the
ci-noble
branch
5 times, most recently
from
3618848
to
957c746
Compare
We don't need a docker image anymore now that we're depending on 24.04
| var msgFormat string | ||
| switch sessionMode { | ||
| case authd.SessionMode_AUTH: | ||
| msgFormat = pam_test.RunnerResultActionAuthenticateFormat | ||
| case authd.SessionMode_PASSWD: | ||
| msgFormat = pam_test.RunnerResultActionChangeAuthTokFormat | ||
| default: | ||
| t.Errorf("Unsupported mode %s", sessionMode) | ||
| } | ||
|
|
||
| sessionMsg := fmt.Sprintf(msgFormat, user) | ||
| if user == "" { | ||
| sessionMsg = strings.ReplaceAll(msgFormat, "%q", "") | ||
| } |
There was a problem hiding this comment.
I don't like that we reimplement assembling of that message here. This will break if anything changes in https://github.com/3v1n0/authd/blob/572504e1e0c10d14d2e2afb89018023fb5c3c04f/pam/tools/pam-runner/pam-runner.go#L118-L133. Can we extract a function like this:
func PamResultMessage(mode string, user string) string {
switch mode {
case "login":
return fmt.Sprintf("PAM ChangeAuthTok() for user %q", user)
case "passwd":
return fmt.Sprintf("PAM Authenticate() for user %q", user)
default:
panic("Invalid PAM operation: " + mode)
}
}
and use it in both places?
There was a problem hiding this comment.
I had done that exactly to avoid breakage, but ok we can instead have a function instead I think, under the pam_test namespace.
There was a problem hiding this comment.
I had done that exactly to avoid breakage
What do you mean? That you reused the pam_test.RunnerResultActionChangeAuthTokFormat and pam_test.RunnerResultActionAuthenticateFormat constants instead of copying the string literal? That's a good first step, but still quite brittle IMO.
There was a problem hiding this comment.
Ah, I guess you mean 742b9c6 where you extracted the constants
There was a problem hiding this comment.
Yeah, let me reconsider your approach... Will handle ti soon :)
| require.Empty(t, os.Getenv("GITHUB_REPOSITORY"), | ||
| "Golden files needs to be updated to ensure CI runs on Ubuntu %v") |
There was a problem hiding this comment.
| require.Empty(t, os.Getenv("GITHUB_REPOSITORY"), | |
| "Golden files needs to be updated to ensure CI runs on Ubuntu %v") | |
| require.Empty(t, os.Getenv("GITHUB_REPOSITORY"), | |
| "Golden files need to be updated to run tests on Ubuntu %v", uv) |
| @@ -257,13 +257,11 @@ jobs: | |||
| echo "Running PAM integration tests" | |||
| pushd ./pam/integration-tests | |||
| go test -asan -gcflags=all="${GO_GC_FLAGS}" -c | |||
| # FIXME: Suppression may be removed with newer libpam, as the one we ship in ubuntu as some leaks | |||
There was a problem hiding this comment.
Great that you thought about this!
|
I'm done with the review, everything looks good beside the small thing I remarked in #641 (comment) |
That's the ubuntu version we're targetting so far, and so we should care
about having it working as expected when doing integration tests in
particular.
Regenerate the SSH golden files to match post-noble SSH server expectations.
UDENG-5316