@AntaresS Why these changes in pkg/term?

Both old and new API have the same status code here?

formatting of else - needs a space

same here (space)

Wonder if we should actually backport the fix for this status; 500 may be due to a bug / not properly handled condition

@AntaresS agree that we should look at what the old test was testing, I doubt we were testing for 500.

@thaJeztah Yeah checked the history of that change - ebcb7d6#diff-ca97ecb464e792b1086fca2ee8af23f8L676
It was not a bug fix but essentially a big change in API error returning code. So we are fine on this.

Hm, right, it's a bit tricky, because it's effectively not related to the API version, but to the version of the daemon that's running (i.e., the "internal server error" was caused by an unhandled error condition, so defaulted to a 500). i.e. using API version 1.31 (or older) on a current version of the daemon would still have the fix, so will return http.StatusBadRequest.

Was thinking if we should do a statuscode != 200 or status code >= 400, but then we'd miss possible regressions if a 500 is returned on the current codebase.

I tend to agree with @thaJeztah… if it's not related to the API version we shouldn't check the API version.. We could also check the dockerd version number (or the version field in the info endpoint for a cleaner way of doing the check) and act based on that ?

@vdemeester @thaJeztah how about we add a TODO explaining the problem. I agree with both of you, there's just not a great way of doing order comparisons between dockerd versions. We can find a proper fix in a followup?

@AntaresS ok let's fix this by only asserting the correct error, if API ≥ 1.32.

Discussing with @tiborvass ; perhaps we should do a "negative" check to be more factual; basically, we want to check that the request didn't succeed / wasn't successful, so, e.g.:

c.Assert(res.StatusCode, checker.Not(checker.Equals), http.StatusOK)

or perhaps check for > 404

(In a follow-up: adding a TODO for now SGTM)

@thaJeztah @tiborvass OK here is what I did. Let's check the correct status code for only API ≥ 1.32 for now. I added TODO for future reference.

if versions.GreaterThanOrEqualTo(testEnv.DaemonAPIVersion(), "1.32") {
      c.Assert(res.StatusCode, checker.Equals, http.StatusBadRequest)
} else {
      c.Assert(res.StatusCode, checker.Not(checker.Equals), http.StatusOK)
}

same here (space)

same here

same here

same here (couple more below)

may want to look at backporting the fix

@thaJeztah not in this PR

could be a onliner :

skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.31"), "new feature added in 1.31, go version `s", testEnv.DaemonAPIVersion())

(not even sure we need to print the current version)

Ah, yes that's better than the testRequires() 👍

oh right but it's with check so might not work 😭

@vdemeester bingo. skip.If won't work here. They use two different interfaces.

@AntaresS yep sorry about that, was thinking it was on integration tests 😛

I tend to agree with @thaJeztah… if it's not related to the API version we shouldn't check the API version.. We could also check the dockerd version number (or the version field in the info endpoint for a cleaner way of doing the check) and act based on that ?

Same as above for oneliner 👼

Same as the reply above 😸

Could be;

testRequires(c, MinimumAPIVersion("1.34"))

Is it API based or docker daemon version dependent ?

We could have only one assertion and just check and have the Err in a variable 👼

👍

Same as above for oneliner 👼

😓

@vdemeester lolz..

nit: should this be testRequires(c, DaemonIsLinux, MinimumAPIVersion("1.31")) ? (looks like that's the approach taken in other locations)

👍

This seems a bit brittle (was it only in 18.05, or also in 18.04?). Note that 18.05 was an edge release, and reaches EOL soon, so we should remove this after that

@thaJeztah Yes. I tend to treat this as a temporary skip check. Because what really happened was this test case was modified 10 days ago according to a bug fix (in 18.06), and it was not backported to 18.05.0. Therefore, ce-test in the e2e test matrix will fail but not ce-nightly.
I'll update this with a todo comment and remove this condition once the versions push forward.

Would it be sufficient to check for invalid container format container:<name|id>" ?

@thaJeztah That's actually true. Now I wonder why it was even changed from invalid to Invalid.
Aren't we following the error string convention -https://github.com/golang/go/wiki/CodeReviewComments#error-strings.
But the changes were made in #33320.

Good question; we should probably change that back

stray blank line here

👍

nit: should this be testRequires(c, DaemonIsLinux, MinimumAPIVersion("1.31")) ? (looks like that's the approach taken in other locations)

👍

This looks odd; This looks like we're testing if a bug is present. I'd rather skip a test than testing bugs

@AntaresS would you mind adding an explanatory comment for the negative value, both here...

@tiborvass yes.

See above re: testing for bugs

@AntaresS ... and here.

Perhaps

if versions.GreaterThan(testEnv.DaemonAPIVersion(), "1.35") && testEnv.OSType != "windows" {
	// The ordering here is due to `PATH` being overridden from the container's
	// ENV.  On windows, the container doesn't have a `PATH` ENV variable so
	// the ordering is the same as the cli.
	expectedEnv = "[PATH=/foo DEBUG=true test=1]"
}

👍

Slightly less DRY:

res, body, err := request.Post("/containers/"+name+"/start", request.JSONBody(config))
c.Assert(err, checker.IsNil)
c.Assert(res.StatusCode, checker.Equals, http.StatusBadRequest)

if versions.GreaterThanOrEqualTo(testEnv.DaemonAPIVersion(), "1.32") {
	// assertions below won't work before 1.32
	buf, err := request.ReadBody(body)
	c.Assert(err, checker.IsNil)
	c.Assert(string(buf), checker.Contains, "was deprecated since API v1.22")
}

👍

Curious: SUSE has /sys/module/apparmor/parameters/enabled, but we consider Apparmor to not be present?

@thaJeztah yes. I asked @andrewhsu and it seems that we don't really support apparmor stuff on SUSE.

In that case; should we disable it on the host?

@thaJeztah In theory, yes, I need to reach out Rusell to disable it when testkit provision the testing environment.

@thaJeztah I agree it's not ideal. Maybe we could check the existence of the apparmor profile for docker instead of just checking whether apparmor is enabled. Do you think we can do that in a followup?

use strings.ToLower() here

@AntaresS little typo. We should also explain that this will check that the status is NOT equal to http.StatusCreated.

Done.