Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for Windows #956

Merged
merged 4 commits into from
Feb 14, 2023
Merged

Add support for Windows #956

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8dc08d1
Add testing stage on Windows
jillguyonnet Feb 8, 2023
2a99b9d
Fix Windows path compatibility
jillguyonnet Feb 9, 2023
42c6291
Implement feedback
jillguyonnet Feb 14, 2023
f159f9a
Additional feedback
jillguyonnet Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 34 additions & 12 deletions .ci/Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,42 @@ pipeline {
Execute unit tests.
*/
stage('Test') {
steps {
cleanup()
withMageEnv(){
dir("${BASE_DIR}"){
sh(label: 'Runs the (unit) tests',script: 'mage -debug test|tee tests-report.txt')
parallel {
stage('Test on Linux') {
options { skipDefaultCheckout() }
steps {
cleanup()
withMageEnv(){
dir("${BASE_DIR}"){
sh(label: 'Runs the (unit) tests',script: 'mage -debug test|tee tests-report.txt')
}
}
}
}
post {
always {
convertGoTestResults(
input: "${BASE_DIR}/tests-report.txt",
output: "${BASE_DIR}/junit-report.xml"
)
}
}
post {
always {
convertGoTestResults(
input: "${BASE_DIR}/tests-report.txt",
output: "${BASE_DIR}/junit-report.xml"
)
}
stage('Test on Windows') {
agent { label 'windows-2022' }
options { skipDefaultCheckout() }
steps {
cleanup()
dir("${BASE_DIR}"){
withGoEnv() {
goTestJUnit(options: '-v ./...', output: 'junit-report.xml')
}
}
}
post {
always {
junit(allowEmptyResults: true, keepLongStdio: true, testResults: "${BASE_DIR}/junit-report.xml")
}
}
}
}
}
Expand Down
7 changes: 5 additions & 2 deletions main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ func TestAllPackageIndex(t *testing.T) {
// find all manifests
var manifests []string
for _, path := range packagesBasePaths {
m, err := filepath.Glob(path + "/*/*/manifest.yml")
m, err := filepath.Glob(filepath.Join(path, "*", "*", "manifest.yml"))
require.NoError(t, err)
manifests = append(manifests, m...)
}
Expand Down Expand Up @@ -574,7 +574,10 @@ func listArchivedFiles(t *testing.T, body []byte) []byte {
var listing bytes.Buffer

for _, f := range zipReader.File {
listing.WriteString(fmt.Sprintf("%d %s\n", f.UncompressedSize64, f.Name))
// f.Name is populated from the zip file directly and is not validated for correctness.
// Using filepath.ToSlash(f.Name) ensures that the file name has the expected format
// regardless of the OS.
listing.WriteString(fmt.Sprintf("%d %s\n", f.UncompressedSize64, filepath.ToSlash(f.Name)))
jsoriano marked this conversation as resolved.
Show resolved Hide resolved

}
return listing.Bytes()
Expand Down
52 changes: 26 additions & 26 deletions packages/datastream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"encoding/json"
"fmt"
"os"
"path/filepath"
"path"
"strings"

"github.com/pkg/errors"
Expand Down Expand Up @@ -111,23 +111,23 @@ type fieldEntry struct {
}

func NewDataStream(basePath string, p *Package) (*DataStream, error) {
fs, err := p.fs()
fsys, err := p.fs()
if err != nil {
return nil, err
}
defer fs.Close()
defer fsys.Close()

manifestPath := filepath.Join(basePath, "manifest.yml")
manifestPath := path.Join(basePath, "manifest.yml")

// Check if manifest exists
_, err = fs.Stat(manifestPath)
_, err = fsys.Stat(manifestPath)
if err != nil && os.IsNotExist(err) {
return nil, errors.Wrapf(err, "manifest does not exist for data stream: %s", p.BasePath)
}

dataStreamPath := filepath.Base(basePath)
dataStreamPath := path.Base(basePath)

b, err := ReadAll(fs, manifestPath)
b, err := ReadAll(fsys, manifestPath)
if err != nil {
return nil, errors.Wrapf(err, "failed to read manifest: %s", err)
}
Expand Down Expand Up @@ -177,16 +177,16 @@ func NewDataStream(basePath string, p *Package) (*DataStream, error) {
return nil, fmt.Errorf("invalid release: %q", d.Release)
}

pipelineDir := filepath.Join(d.BasePath, "elasticsearch", DirIngestPipeline)
paths, err := fs.Glob(filepath.Join(pipelineDir, "*"))
pipelineDir := path.Join(d.BasePath, "elasticsearch", DirIngestPipeline)
paths, err := fsys.Glob(path.Join(pipelineDir, "*"))
if err != nil {
return nil, err
}

if d.Elasticsearch != nil && d.Elasticsearch.IngestPipelineName == "" {
// Check that no ingest pipeline exists in the directory except default
for _, path := range paths {
if filepath.Base(path) == DefaultPipelineNameJSON || filepath.Base(path) == DefaultPipelineNameYAML {
for _, p := range paths {
if path.Base(p) == DefaultPipelineNameJSON || path.Base(p) == DefaultPipelineNameYAML {
d.Elasticsearch.IngestPipelineName = DefaultPipelineName
// TODO: remove because of legacy
d.IngestPipeline = DefaultPipelineName
Expand All @@ -196,8 +196,8 @@ func NewDataStream(basePath string, p *Package) (*DataStream, error) {
// TODO: Remove, only here for legacy
} else if d.IngestPipeline == "" {
// Check that no ingest pipeline exists in the directory except default
for _, path := range paths {
if filepath.Base(path) == DefaultPipelineNameJSON || filepath.Base(path) == DefaultPipelineNameYAML {
for _, p := range paths {
if path.Base(p) == DefaultPipelineNameJSON || path.Base(p) == DefaultPipelineNameYAML {
d.IngestPipeline = DefaultPipelineName
break
}
Expand All @@ -222,37 +222,37 @@ func (d *DataStream) Validate() error {
return fmt.Errorf("type is not valid: %s", d.Type)
}

fs, err := d.packageRef.fs()
fsys, err := d.packageRef.fs()
if err != nil {
return err
}
defer fs.Close()
defer fsys.Close()

// In case an ingest pipeline is set, check if it is around
pipelineDir := filepath.Join(d.BasePath, "elasticsearch", DirIngestPipeline)
pipelineDir := path.Join(d.BasePath, "elasticsearch", DirIngestPipeline)
if d.IngestPipeline != "" {
var validFound bool

jsonPipelinePath := filepath.Join(pipelineDir, d.IngestPipeline+".json")
_, errJSON := fs.Stat(jsonPipelinePath)
jsonPipelinePath := path.Join(pipelineDir, d.IngestPipeline+".json")
_, errJSON := fsys.Stat(jsonPipelinePath)
if errJSON != nil && !os.IsNotExist(errJSON) {
return errors.Wrapf(errJSON, "stat ingest pipeline JSON file failed (path: %s)", jsonPipelinePath)
}
if !os.IsNotExist(errJSON) {
err := validateIngestPipelineFile(fs, jsonPipelinePath)
err := validateIngestPipelineFile(fsys, jsonPipelinePath)
if err != nil {
return errors.Wrapf(err, "validating ingest pipeline JSON file failed (path: %s)", jsonPipelinePath)
}
validFound = true
}

yamlPipelinePath := filepath.Join(pipelineDir, d.IngestPipeline+".yml")
_, errYAML := fs.Stat(yamlPipelinePath)
yamlPipelinePath := path.Join(pipelineDir, d.IngestPipeline+".yml")
_, errYAML := fsys.Stat(yamlPipelinePath)
if errYAML != nil && !os.IsNotExist(errYAML) {
return errors.Wrapf(errYAML, "stat ingest pipeline YAML file failed (path: %s)", jsonPipelinePath)
}
if !os.IsNotExist(errYAML) {
err := validateIngestPipelineFile(fs, yamlPipelinePath)
err := validateIngestPipelineFile(fsys, yamlPipelinePath)
if err != nil {
return errors.Wrapf(err, "validating ingest pipeline YAML file failed (path: %s)", jsonPipelinePath)
}
Expand All @@ -264,7 +264,7 @@ func (d *DataStream) Validate() error {
}
}

err = d.validateRequiredFields(fs)
err = d.validateRequiredFields(fsys)
if err != nil {
return errors.Wrap(err, "validating required fields failed")
}
Expand All @@ -282,7 +282,7 @@ func validateIngestPipelineFile(fs PackageFileSystem, pipelinePath string) error
return errors.Wrapf(err, "reading ingest pipeline file failed (path: %s)", pipelinePath)
}

ext := filepath.Ext(pipelinePath)
ext := path.Ext(pipelinePath)
var m map[string]interface{}
switch ext {
case ".json":
Expand All @@ -297,10 +297,10 @@ func validateIngestPipelineFile(fs PackageFileSystem, pipelinePath string) error

// validateRequiredFields method loads fields from all files and checks if required fields are present.
func (d *DataStream) validateRequiredFields(fs PackageFileSystem) error {
fieldsDirPath := filepath.Join(d.BasePath, "fields")
fieldsDirPath := path.Join(d.BasePath, "fields")

// Collect fields from all files
fieldsFiles, err := fs.Glob(filepath.Join(fieldsDirPath, "*"))
fieldsFiles, err := fs.Glob(path.Join(fieldsDirPath, "*"))
if err != nil {
return err
}
Expand Down
70 changes: 47 additions & 23 deletions packages/fs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"io"
"io/fs"
"os"
"path"
"path/filepath"
"strings"
)
Expand All @@ -28,31 +29,52 @@ type PackageFileSystem interface {
// ExtractedPackageFileSystem provides utils to access files in an extracted package.
type ExtractedPackageFileSystem struct {
path string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

root fs.FS
}

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

func (fs *ExtractedPackageFileSystem) Stat(name string) (os.FileInfo, error) {
return os.Stat(filepath.Join(fs.path, name))
func (fsys *ExtractedPackageFileSystem) Stat(name string) (os.FileInfo, error) {
name = strings.TrimPrefix(name, "/")
return fs.Stat(fsys.root, name)
}

func (fs *ExtractedPackageFileSystem) Open(name string) (PackageFile, error) {
return os.Open(filepath.Join(fs.path, name))
}

func (fs *ExtractedPackageFileSystem) Glob(pattern string) (matches []string, err error) {
matches, err = filepath.Glob(filepath.Join(fs.path, pattern))
func (fsys *ExtractedPackageFileSystem) Open(name string) (PackageFile, error) {
name = strings.TrimPrefix(name, "/")
f, err := fsys.root.Open(name)
if err != nil {
return
return nil, err
}
for i := range matches {
match, err := filepath.Rel(fs.path, matches[i])
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
// f is a plain os.File expected to implement the PackageFile interface.
pf, ok := f.(PackageFile)
Copy link
Member

@jsoriano jsoriano Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if !ok {
defer f.Close()
return nil, fmt.Errorf("file does not implement PackageFile interface: %q", filepath.Join(fsys.path, filepath.FromSlash(name)))
}
return pf, nil
}

func (fsys *ExtractedPackageFileSystem) Glob(pattern string) (matches []string, err error) {
e := fs.WalkDir(fsys.root, ".", func(p string, d fs.DirEntry, err error) error {
jillguyonnet marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, fmt.Errorf("failed to obtain path under package root path (%s): %w", fs.path, err)
// Path related error: returning it will cause WalkDir to stop walking the entire tree.
return fmt.Errorf("failed to walk path %q: %w", p, err)
}
match, err := path.Match(pattern, p)
if err != nil {
// ErrBadPattern error: returning it will cause WalkDir to stop walking the entire tree.
return fmt.Errorf("failed to match path %q against pattern %s: %w", p, pattern, err)
}
if match {
name := p
matches = append(matches, name)
}
matches[i] = match
return nil
})
if e != nil {
return nil, fmt.Errorf("failed to obtain path under package root path (%s): %w", pattern, e)
}
return
}
Expand All @@ -73,10 +95,12 @@ func NewZipPackageFileSystem(p *Package) (*ZipPackageFileSystem, error) {
var root string
found := false
for _, f := range reader.File {
name := filepath.Clean(f.Name)
parts := strings.Split(name, string(filepath.Separator))
if len(parts) == 2 && parts[1] == "manifest.yml" {
root = parts[0]
name := path.Clean(f.Name)
dir, base := path.Split(name)
dir = path.Clean(dir)
// Find manifest files at the root of the package
if dir != "." && path.Dir(dir) == "." && base == "manifest.yml" {
root = dir
found = true
jillguyonnet marked this conversation as resolved.
Show resolved Hide resolved
break
}
Expand All @@ -91,7 +115,7 @@ func NewZipPackageFileSystem(p *Package) (*ZipPackageFileSystem, error) {
}

func (fs *ZipPackageFileSystem) Stat(name string) (os.FileInfo, error) {
path := filepath.Join(fs.root, name)
path := path.Join(fs.root, name)
f, err := fs.reader.Open(path)
if err != nil {
return nil, err
Expand All @@ -101,7 +125,7 @@ func (fs *ZipPackageFileSystem) Stat(name string) (os.FileInfo, error) {
}

func (fs *ZipPackageFileSystem) Open(name string) (PackageFile, error) {
path := filepath.Join(fs.root, name)
path := path.Join(fs.root, name)
f, err := fs.reader.Open(path)
if err != nil {
return nil, err
Expand All @@ -114,14 +138,14 @@ func (fs *ZipPackageFileSystem) Open(name string) (PackageFile, error) {
}

func (fs *ZipPackageFileSystem) Glob(pattern string) (matches []string, err error) {
pattern = filepath.Join(fs.root, pattern)
pattern = path.Join(fs.root, pattern)
for _, f := range fs.reader.File {
match, err := filepath.Match(pattern, filepath.Clean(f.Name))
match, err := path.Match(pattern, path.Clean(f.Name))
if err != nil {
return nil, err
}
if match {
name := strings.TrimPrefix(f.Name, fs.root+string(filepath.Separator))
name := strings.TrimPrefix(f.Name, fs.root+"/")
matches = append(matches, name)
}
}
Expand Down
Loading