Add support for Windows #956

jillguyonnet · 2023-02-08T09:00:34Z

Issue description

EPR currently fails to run on Windows due to the use of filepath library in an abstracted file system.

What does this PR do?

Add a Jenkins testing stage on Windows
Fix Windows compatiblity

Why is it important?

EPR currently cannot run on Windows.

Related issues

Closes Windows support #955

elasticmachine · 2023-02-08T09:05:57Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-02-14T15:13:02.325+0000
Duration: 11 min 4 sec

Test stats 🧪

Test	Results
Failed	0
Passed	488
Skipped	0
Total	488

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

.ci/Jenkinsfile

jsoriano · 2023-02-09T14:33:29Z

packages/fs.go

@@ -35,23 +36,29 @@ func NewExtractedPackageFileSystem(p *Package) (*ExtractedPackageFileSystem, err
 }

 func (fs *ExtractedPackageFileSystem) Stat(name string) (os.FileInfo, error) {
-	return os.Stat(filepath.Join(fs.path, name))
+	return os.Stat(path.Join(fs.path, name))


I think that when accessing the file system directly, we should keep using filepath, that takes into account the different separator in Windows. This would be for all methods of ExtractedPackageFileSystem, that work on the native file system.

As a rule of thumb, if the path is used with a function of os package, it should be manipulated with filepath package.

If we want to work with path, we could use os.DirFS to build a fs.FS.

packages/fs.go

jillguyonnet · 2023-02-09T15:06:23Z

Hi @jsoriano thanks for the tips, I'm on a learning curve so appreciate it!

jsoriano · 2023-02-13T08:51:41Z

Hey @jillguyonnet,

I am thinking that it could be easier to fix the pending issues if we use path everywhere, as you were originally doing. This would probably solve a couple of issues:

The paths in the API that are currently causing the tests to fail.
Methods of the ExtractedPackageFileSystem type would always work with POSIX paths, also when running on Windows.

For that, we could create the ExtractedPackageFileSystems this way:

type ExtractedPackageFileSystem struct {
	path string
	root fs.FS
}

func NewExtractedPackageFileSystem(p *Package) (*ExtractedPackageFileSystem, error) {
	return &ExtractedPackageFileSystem{path: p.BasePath, root: os.DirFS(p.BasePath)}, nil
}

And we would make all operations with path and fs methods over root.

jillguyonnet · 2023-02-14T11:22:54Z

/test

jsoriano

Good job here, there were several tricky cases! Added some nitpicking.

I think the only thing I would like to see reviewed before merging would be to handle the error of WalkDir, and to include the fsys.path in error messages. For the rest it LGTM.

main_test.go

jsoriano · 2023-02-14T13:39:50Z

packages/datastream.go

-	pipelineDir := filepath.Join(d.BasePath, "elasticsearch", DirIngestPipeline)
-	paths, err := fs.Glob(filepath.Join(pipelineDir, "*"))
+	pipelineDir := path.Join(d.BasePath, "elasticsearch", DirIngestPipeline)
+	paths, err := fs.Glob(path.Join(pipelineDir, "*"))


Nit. Maybe we should start using fsys in general as abbreviation for filesystems to distingish from fs package.

jsoriano · 2023-02-14T13:41:10Z

packages/fs.go

+}
+
+func (fsys *ExtractedPackageFileSystem) Glob(pattern string) (matches []string, err error) {
+	fs.WalkDir(fsys.root, ".", func(p string, d fs.DirEntry, err error) error {


If I recall correctly, WalkDir returns an error, we should handle it.

jsoriano · 2023-02-14T13:42:07Z

packages/fs.go

@@ -28,32 +28,47 @@ type PackageFileSystem interface {
 // ExtractedPackageFileSystem provides utils to access files in an extracted package.
 type ExtractedPackageFileSystem struct {
 	path string


I think the only reason to keep the path now here would be to use it in error messages so users know to what package in the filesystem we are referring to. Use it when displaying paths in error messages of this package.

jsoriano · 2023-02-14T13:43:41Z

packages/fs.go

 	}
-	for i := range matches {
-		match, err := filepath.Rel(fs.path, matches[i])
+	pf, ok := f.(PackageFile)


Please add a comment here mentioning that we expect this to be a plain os.File, that implements this interface. It may look a bit random to convert to this interface otherwise.

jsoriano · 2023-02-14T13:45:25Z

packages/fs.go

+		defer f.Close()
+		return nil, fmt.Errorf("file does not implement PackageFile interface: %q", name)


We could think to even panic() here, as all the files here should actually implement PackageFile. But let's stay on the safer side and keep it as an error.

jsoriano · 2023-02-14T13:49:40Z

packages/fs.go

+		defer f.Close()
+		return nil, fmt.Errorf("file does not implement PackageFile interface: %q", name)


This is a place where we may want to use fsys.path to display the full path to the user. Something like this:

Suggested change

defer f.Close()

return nil, fmt.Errorf("file does not implement PackageFile interface: %q", name)

defer f.Close()

return nil, fmt.Errorf("file does not implement PackageFile interface: %q", filepath.Join(f.path, filepath.FromSlash(name)))

If we do this on multiple places, you may consider to implement a helper method for that, something like this:

func (fsys *ExtractedPackageFileSystem) fullPath(name string) (string) { return filepath.Join(f.path, filepath.FromSlash(name)) }

jillguyonnet · 2023-02-14T14:53:53Z

@jsoriano Thanks for the review, I addressed your comments. Very good point about fsys.path. I edited a bit the error messages inside

func (fsys *ExtractedPackageFileSystem) Glob(pattern string) (matches []string, err error)

and the only place I can see that might make use of fsys.path would be the final error returned by WalkDir, so currently:

fmt.Errorf("failed to obtain path under package root path (%s): %w", pattern, e)

I suppose there could also be a custom error message in the Open method.

Can you please let me know your thoughts on this and of anything else I might have missed? Thanks!

packages/fs.go

mrodm

Great job here!

jillguyonnet force-pushed the jillguyonnet/add-windows-support branch from 8e593de to e76bc7c Compare February 8, 2023 09:08

mrodm reviewed Feb 8, 2023

View reviewed changes

.ci/Jenkinsfile Outdated Show resolved Hide resolved

jillguyonnet force-pushed the jillguyonnet/add-windows-support branch 3 times, most recently from 2430bb0 to 859c27f Compare February 9, 2023 09:25

jsoriano reviewed Feb 9, 2023

View reviewed changes

jillguyonnet force-pushed the jillguyonnet/add-windows-support branch from 369aedf to e896116 Compare February 10, 2023 15:18

jillguyonnet added 2 commits February 14, 2023 14:28

Add testing stage on Windows

8dc08d1

Fix Windows path compatibility

2a99b9d

jillguyonnet force-pushed the jillguyonnet/add-windows-support branch from 8d91a91 to 2a99b9d Compare February 14, 2023 13:28

jillguyonnet marked this pull request as ready for review February 14, 2023 13:36

jsoriano reviewed Feb 14, 2023

View reviewed changes

Implement feedback

42c6291

jsoriano approved these changes Feb 14, 2023

View reviewed changes

packages/fs.go Outdated Show resolved Hide resolved

packages/fs.go Outdated Show resolved Hide resolved

Additional feedback

f159f9a

jsoriano approved these changes Feb 14, 2023

View reviewed changes

mrodm approved these changes Feb 14, 2023

View reviewed changes

jillguyonnet merged commit 70faae3 into elastic:main Feb 14, 2023

jillguyonnet deleted the jillguyonnet/add-windows-support branch February 14, 2023 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Windows #956

Add support for Windows #956

jillguyonnet commented Feb 8, 2023 •

edited by jlind23

Loading

elasticmachine commented Feb 8, 2023 •

edited

Loading

Build stats

Test stats 🧪

jsoriano Feb 9, 2023

jsoriano Feb 9, 2023

jillguyonnet commented Feb 9, 2023

jsoriano commented Feb 13, 2023

jillguyonnet commented Feb 14, 2023

jsoriano left a comment

jsoriano Feb 14, 2023

jsoriano Feb 14, 2023

jsoriano Feb 14, 2023

jsoriano Feb 14, 2023 •

edited

Loading

jsoriano Feb 14, 2023

jsoriano Feb 14, 2023

jillguyonnet commented Feb 14, 2023

mrodm left a comment

		defer f.Close()
		return nil, fmt.Errorf("file does not implement PackageFile interface: %q", name)

Add support for Windows #956

Add support for Windows #956

Conversation

jillguyonnet commented Feb 8, 2023 • edited by jlind23 Loading

Issue description

What does this PR do?

Why is it important?

Related issues

elasticmachine commented Feb 8, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

jsoriano Feb 9, 2023

Choose a reason for hiding this comment

jsoriano Feb 9, 2023

Choose a reason for hiding this comment

jillguyonnet commented Feb 9, 2023

jsoriano commented Feb 13, 2023

jillguyonnet commented Feb 14, 2023

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Feb 14, 2023

Choose a reason for hiding this comment

jsoriano Feb 14, 2023

Choose a reason for hiding this comment

jsoriano Feb 14, 2023

Choose a reason for hiding this comment

jsoriano Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

jsoriano Feb 14, 2023

Choose a reason for hiding this comment

jsoriano Feb 14, 2023

Choose a reason for hiding this comment

jillguyonnet commented Feb 14, 2023

mrodm left a comment

Choose a reason for hiding this comment

jillguyonnet commented Feb 8, 2023 •

edited by jlind23

Loading

elasticmachine commented Feb 8, 2023 •

edited

Loading

jsoriano Feb 14, 2023 •

edited

Loading