-
Notifications
You must be signed in to change notification settings - Fork 147
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
clean up configuration handling #233
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
os.Exit(m.Run()) | ||
} | ||
|
||
func TestSanity(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering whether we need this special hack with csi-sanity
being a go test
program. It prevents using "go get" to build it. I might still change that as part of this PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was pretty simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #234.
/assign @misterikkit |
d53f998
to
d5badbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! I forgot to send these draft comments. If they no longer make sense, feel free to ignore. I will take another pass at reviewing this soon.
pkg/sanity/sanity.go
Outdated
CreatePathCmdTimeout: 10, | ||
RemovePathCmdTimeout: 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these values in seconds? Any reason it's not time.Duration
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Historic reasons. I can change it.
pkg/sanity/sanity.go
Outdated
StagingPath: os.TempDir() + "/csi-staging", | ||
CreatePathCmdTimeout: 10, | ||
RemovePathCmdTimeout: 10, | ||
TestVolumeSize: 10 * 1024 * 1024 * 1024, // 10 GB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 10 GiB
. Could you update the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
// the instance to [Ginkgo]Test and/or (when using GinkgoTest) in a | ||
// BeforeEach. For example, the BeforeEach could set up the CSI driver | ||
// and then set the Address field differently for each test. | ||
type TestConfig struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the struct name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, this is GitHub, so no-one reads commit messages, but I write them anyway... 😬
In the commit message, I usually explain the "why" for a change ("why do we need it, why this way") if it doesn't fit into a comment. In this case, the explanation is: "To ensure that callers adapt to the
new semantic, the struct gets renamed. ... SanityContext gets renamed to TestContext for consistency."
Another reason (not called out specifically) is that sanity.SanityConfig
stuttered. sanity.TestConfig
is a better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to answer this twice. 😄
// BeforeEach. For example, the BeforeEach could set up the CSI driver | ||
// and then set the Address field differently for each test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a godoc example of how one would use this struct. That's enough of a change that it doesn't need to be in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's overkill. I find it more likely that someone starts using this package based on some existing CSI driver, i.e. with a working example, than starting just with the godocs. I could be wrong, of course.
@misterikkit : PR updated, please have another look. |
NewTestConfig now must be used to initialize the struct with defaults. This makes it easier to introduce new fields where the empty value isn't a suitable default. To ensure that callers adapt to the new semantic, the struct gets renamed. This also allows removing replicated default values all over the source code. SanityContext gets renamed to TestContext for consistency. Its new Finalize method should be used to clean up. To allow that, GinkgoTest returns the context pointer.
This is cleaner. This also changes the values of the command line timeout parameters from plain int (10) to durations (10s), but this is okay as we are preparing a major new release.
81bc939
to
90f9d07
Compare
@misterikkit: rebased on top of the recently merged #236. @wnxn: perhaps you can have a look at this PR and verify that I didn't break anything? |
I verified it. It didn't break anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
"testing" | ||
|
||
"github.com/kubernetes-csi/csi-test/pkg/sanity" | ||
. "github.com/onsi/ginkgo" | ||
. "github.com/onsi/gomega" | ||
) | ||
|
||
var context *sanity.TestContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a global is the best way to do this within Ginkgo. Oh well.
// the instance to [Ginkgo]Test and/or (when using GinkgoTest) in a | ||
// BeforeEach. For example, the BeforeEach could set up the CSI driver | ||
// and then set the Address field differently for each test. | ||
type TestConfig struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to answer this twice. 😄
// GinkgoTest for use when the tests run. Therefore its content can | ||
// still be modified in a BeforeEach. The sanity package itself treats | ||
// it as read-only. | ||
func GinkgoTest(config *TestConfig) *TestContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe RegisterGinkgoTests
would be a good name for this, but we seem to have enough breaking changes on our hands.
remove windows 20H2 build since it's EOL long time ago
What type of PR is this?
/kind api-change
What this PR does / why we need it:
As discussed in #220, the current API makes it hard to introduce new config fields which don't have the nul value as default. The new
NewTestConfig
addresses that.As were are breaking the API with that already, several other shortcomings can also be addressed.
Special notes for your reviewer:
This is meant to go into v3.0.0 together with PR #232 .
/cc @misterikkit @davidz627
Does this PR introduce a user-facing change?: