chore: add windows tests #120

eddycharly · 2024-04-17T20:00:00Z

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

eddycharly · 2024-04-18T08:46:32Z

/label tide/merge-method-squash

eddycharly · 2024-04-18T10:23:25Z

pkg/openapiclient/local_schemas.go

-		versions, err := utils.ReadDir(k.fs, groupPath)
-		if err != nil {
-			return nil, fmt.Errorf("failed reading local files dir %s: %w", groupPath, err)
+	if apiGroups, err := fs.ReadDir(k.fs, "apis"); err == nil {


note that the behaviour changes slightly. this is because it seems impossible to have a consistent behaviour between windows and linux :(

see golang/go#46734

IIUC the go issue you linked refers to files opened with ReadDir returning true for errors.Is(err, fs.ErrNotExist)

That would mean for files with the name apis we throw no error and apiGroups is empty. I think that is preferrable to the current diff which throws out all errors.

Was the original code causing problems? If i understood correctly as above I feel like it wouldnt throw any more errors, just throw one less error for the file-named-apis case.

IIRC the inconsistency case was when calling ReadDir on a file.

That would mean for files with the name apis we throw no error and apiGroups is empty. I think that is preferrable to the current diff which throws out all errors.

In this case ReadDir will fail, err == nil will not be true and apiGroups is not considered at all.
My feeling was that we were becoming more permissive with this kind of errors so it made sense to check err == nil and just do nothing in case there was an error.

Let me know if you want me to change the behaviour but yeah the real problem is errors.Is(err, fs.ErrNotExist) not being consistent across all systems (at least with an error we got from ReadDir).

Let me know if you want me to change the behaviour but yeah the real problem is errors.Is(err, fs.ErrNotExist) not being consistent across all systems (at least with an error we got from ReadDir).

Is it possible to check if the file exists without checking the ReadDir err result? Like stat?

apiGroups, err := fs.ReadDir(k.fs, "apis") if err != nil && CrossPlatformCheckDirExists(k.fs, "apis") { return nil, fmt.Errorf("...: %w", err) }

We can probably do something like that.
Ultimately do you prefer returning an error in this case ? Or just skip the error to let the chain continue collecting paths ?

If the dir does not exist it should skip it, but if it does exist and we couldn't read it then it should be an error

alexzielenski

Really appreciate you doing this! I had a few questions

alexzielenski · 2024-04-23T17:37:44Z

pkg/openapiclient/local_schemas.go

-		versions, err := utils.ReadDir(k.fs, groupPath)
-		if err != nil {
-			return nil, fmt.Errorf("failed reading local files dir %s: %w", groupPath, err)
+	if apiGroups, err := fs.ReadDir(k.fs, "apis"); err == nil {


IIUC the go issue you linked refers to files opened with ReadDir returning true for errors.Is(err, fs.ErrNotExist)

That would mean for files with the name apis we throw no error and apiGroups is empty. I think that is preferrable to the current diff which throws out all errors.

Was the original code causing problems? If i understood correctly as above I feel like it wouldnt throw any more errors, just throw one less error for the file-named-apis case.

pkg/openapiclient/local_crds.go

pkg/openapiclient/local_schemas.go

eddycharly · 2024-04-23T18:19:00Z

Really appreciate you doing this!

I think i introduced the bug... I break it, i fix it! =)

pkg/openapiclient/local_crds_test.go

alexzielenski · 2024-04-24T18:03:40Z

pkg/openapiclient/local_schemas_test.go

-		fs:   os.DirFS("../../invalid"),
-		dir:  ".",
+		name: "not a dir",
+		fs:   os.DirFS("../../testcases/schemas/error_not_a_dir"),
 		want: sets.New[string](),


This fails on windows returning a nil set rather than an empty set, but not on *nix.

It is also curious to me that we can return nil Paths() but no error

It is also curious to me that we can return nil Paths() but no error

If there's no api nor apis folders, what shall we return ?

The problem was reading /api / /apis without trying to read / first 🙈

eddycharly · 2024-04-25T08:14:36Z

@alexzielenski tests are green finally :)

alexzielenski

one more nit 😬

alexzielenski · 2024-04-25T18:49:58Z

pkg/openapiclient/local_schemas.go

+func crossPlatformCheckDirExists(f fs.FS, path string) bool {
+	_, err := fs.Stat(f, path)
+	if err != nil {
+		return !os.IsNotExist(err)


I looked at the implementation for os.IsNotExist...it seems very similar to errors.Is except it only looks at specific syscall error types and godoc advises agsinst new usage:

// IsNotExist returns a boolean indicating whether the error is known to // report that a file or directory does not exist. It is satisfied by // ErrNotExist as well as some syscall errors. // // This function predates errors.Is. It only supports errors returned by // the os package. New code should use errors.Is(err, fs.ErrNotExist). func IsNotExist(err error) bool { return underlyingErrorIs(err, ErrNotExist) }

Did the errors.Is check not work here?

trying with errors.Is...

alexzielenski · 2024-04-25T18:59:58Z

pkg/openapiclient/local_schemas.go

+	// check if '.' can be listed
+	if _, err := fs.ReadDir(k.fs, "."); err != nil {
+		if crossPlatformCheckDirExists(k.fs, ".") {
+			return nil, fmt.Errorf("error listing %s: %w", ".", err)
+		}
+	}


I assume this is right but I dont understand why this is necessary. My expectation was that if . cant be listed then ReadDir would yield an error? I'd expect the crossPlatfomrCheckDirExists check after that to not skip the error since it wouldnt be an IsNotExist error, but some permission or something?

My feeling is that if . is a file and we try to list /api it returns a not exist on windows. It's hard to check as i don't have a windows box but i can send a unit test and log the results...

alexzielenski · 2024-04-25T20:04:53Z

/lgtm
/approve

eddycharly · 2024-04-25T20:16:55Z

@alexzielenski Shouldn't tide/merge-method-squash allow merging even with merge commits ?

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

alexzielenski · 2024-04-26T23:04:37Z

/lgtm
/approve

k8s-ci-robot · 2024-04-26T23:04:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, eddycharly

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:

~~OWNERS~~ [alexzielenski]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 17, 2024

k8s-ci-robot requested review from alexzielenski and richabanker April 17, 2024 20:00

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 17, 2024

eddycharly force-pushed the windows-tests branch from bc7b2b1 to ebbdab2 Compare April 17, 2024 20:12

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 17, 2024

eddycharly force-pushed the windows-tests branch from ebbdab2 to b819092 Compare April 17, 2024 20:28

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 17, 2024

eddycharly marked this pull request as draft April 17, 2024 20:38

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 17, 2024

eddycharly force-pushed the windows-tests branch 3 times, most recently from 9a8b1c6 to 727413b Compare April 18, 2024 08:41

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 18, 2024

eddycharly force-pushed the windows-tests branch from 727413b to 2628801 Compare April 18, 2024 08:43

k8s-ci-robot added the do-not-merge/contains-merge-commits label Apr 18, 2024

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 18, 2024

eddycharly force-pushed the windows-tests branch 3 times, most recently from c27bbe7 to 9a44bb1 Compare April 18, 2024 10:19

eddycharly marked this pull request as ready for review April 18, 2024 10:21

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 18, 2024

eddycharly commented Apr 18, 2024

View reviewed changes

alexzielenski reviewed Apr 23, 2024

View reviewed changes

eddycharly force-pushed the windows-tests branch from 9a44bb1 to 1375db5 Compare April 23, 2024 18:37

eddycharly force-pushed the windows-tests branch from 1375db5 to 1e3c55d Compare April 24, 2024 17:28

alexzielenski requested changes Apr 24, 2024

View reviewed changes

eddycharly force-pushed the windows-tests branch 3 times, most recently from f64ac7d to 3bed0bb Compare April 25, 2024 08:05

alexzielenski reviewed Apr 25, 2024

View reviewed changes

eddycharly force-pushed the windows-tests branch from 3bed0bb to b711c2f Compare April 25, 2024 19:34

k8s-ci-robot assigned alexzielenski Apr 25, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 25, 2024

eddycharly force-pushed the windows-tests branch from b711c2f to 1ad4da4 Compare April 26, 2024 22:44

eddycharly added 2 commits April 27, 2024 00:45

chore: add windows tests

d905c9e

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

fix

0bd7a50

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

eddycharly force-pushed the windows-tests branch from 1ad4da4 to 0bd7a50 Compare April 26, 2024 22:47

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 26, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2024

k8s-ci-robot merged commit 57b5e37 into kubernetes-sigs:main Apr 26, 2024
6 checks passed

eddycharly deleted the windows-tests branch April 26, 2024 23:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add windows tests #120

chore: add windows tests #120

eddycharly commented Apr 17, 2024

eddycharly commented Apr 18, 2024

eddycharly Apr 18, 2024

alexzielenski Apr 23, 2024

eddycharly Apr 23, 2024

alexzielenski Apr 23, 2024

eddycharly Apr 23, 2024 •

edited

Loading

alexzielenski Apr 24, 2024

alexzielenski left a comment

alexzielenski Apr 23, 2024

eddycharly commented Apr 23, 2024

alexzielenski Apr 24, 2024

eddycharly Apr 25, 2024

eddycharly Apr 25, 2024

eddycharly commented Apr 25, 2024

alexzielenski left a comment

alexzielenski Apr 25, 2024

eddycharly Apr 25, 2024

alexzielenski Apr 25, 2024 •

edited

Loading

eddycharly Apr 25, 2024

alexzielenski commented Apr 25, 2024

eddycharly commented Apr 25, 2024

alexzielenski commented Apr 26, 2024

k8s-ci-robot commented Apr 26, 2024

chore: add windows tests #120

chore: add windows tests #120

Conversation

eddycharly commented Apr 17, 2024

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

eddycharly commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddycharly Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzielenski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddycharly commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddycharly commented Apr 25, 2024

alexzielenski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzielenski Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzielenski commented Apr 25, 2024

eddycharly commented Apr 25, 2024

alexzielenski commented Apr 26, 2024

k8s-ci-robot commented Apr 26, 2024

eddycharly Apr 23, 2024 •

edited

Loading

alexzielenski Apr 25, 2024 •

edited

Loading