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.

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

feat: Introduce .kraftignore #1032

Open
wants to merge 2 commits into
base: staging
Choose a base branch
from

Conversation

MdSahil-oss
Copy link
Contributor

@MdSahil-oss MdSahil-oss commented Nov 23, 2023

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran make fmt on your commit series before opening this PR;
  • Updated relevant documentation.

Description of changes

This PR fixes #135

Specifying contents (file or directory paths) in .kraftignore file in the working directory forces Kraftkit to ignore matching children files or directories sharing from the sharing directory as rootfs.

For now, .kraftignore only work when rootfs is specified for a directory path.

Rules for .kraftignore:

  • Line starts with a # will be ignored by Kraftkit.

  • Every blank line will be ignored by Kraftkit.

  • Directory or File path that contains glob-patterns (characters like *, !, [ and ?) will be ignored by Kraftkit and return a warning message.

  • File or Directory name can be enclosed by single or double quotes ('filename', "dir_name").

  • File or Directory names which contain white spaces must be enclosed by single or double quotes. e.g.

    "sample file"
    'sample new file'
  • One needs to specify the entire relative path (relative path to the sharing directory e.g. if rootfs=share/ and share directory contains sample.txtand sample/, So relative path in .kraftignore file can be defined as /sample.txt, sample.txt and ./sample.txt for share/sample.txt to ignore) of the file or directory that have to be ignored.

  • Various directory and file paths can be specified in a .kraftignore file using space separator or new line e.g.

    random1/random01/random01.txt /random.txt
    
    random1/random01
  • All the specified paths in .kraftignore will be relative to the root of sharing directory, means if share/ directory contains sample.txt and sample/, and set as rootfs to kraftkit then to ignore /share/sample.txt only sample.txt needs to be defined in kraftignore.
    Note: All the paths can start with ./ or / (Since prefix / and ./ with paths simply ignored by Kraftkit).

@MdSahil-oss MdSahil-oss changed the title WIP: .kraftignore feat: .kraftignore Nov 23, 2023
@MdSahil-oss MdSahil-oss force-pushed the kraftignore branch 2 times, most recently from 47907c8 to 32e420e Compare November 23, 2023 20:09
@MdSahil-oss MdSahil-oss changed the title feat: .kraftignore feat: Adds .kraftignore support (WIP) Nov 26, 2023
@MdSahil-oss MdSahil-oss changed the title feat: Adds .kraftignore support (WIP) feat: Introduce .kraftignore (WIP) Nov 26, 2023
@MdSahil-oss MdSahil-oss force-pushed the kraftignore branch 6 times, most recently from d6461e2 to 9f7f0ce Compare November 30, 2023 19:45
@MdSahil-oss MdSahil-oss changed the title feat: Introduce .kraftignore (WIP) feat: Introduce .kraftignore Dec 1, 2023
Copy link
Member

@craciunoiuc craciunoiuc left a comment

Choose a reason for hiding this comment

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

Some initial comments from my side, did not try out the code yet

initrd/kraftignore.go Outdated Show resolved Hide resolved
initrd/directory.go Outdated Show resolved Hide resolved
initrd/directory.go Outdated Show resolved Hide resolved
initrd/kraftignore.go Outdated Show resolved Hide resolved
initrd/kraftignore.go Outdated Show resolved Hide resolved
initrd/kraftignore.go Outdated Show resolved Hide resolved
@craciunoiuc
Copy link
Member

I think we also need to document this somewhere otherwise it will be lost

@nderjung do you have a use case in mind? maybe an app which would currently benefit from .kraftignore?

@craciunoiuc
Copy link
Member

craciunoiuc commented Jan 16, 2024

Hey @MdSahil-oss found 1 small bug but otherwise it seems to work:

test.txt <-- works (ignored)

# test1.txt <-- works (not ignored)

nginx/test3.txt    #       test2.txt  <-- works (both ignored, basically # was ignored)

test.txt <-- works (nothing changed)

# test.txt <-- works (nothing changed)

./nginx <-- works (ignored)

./nginx/* <-- works (prints warning)

.///////////////////nginx <-- fails (multiple slashes are usually ignored)
./nginx/../test1.txt <-- fails (I think .. is not understood)
././././././././././././test1.txt <-- fails (I think . is not understood)

@craciunoiuc
Copy link
Member

One more thing: does it support using "" or '' characters? I was thinking about the case of "./nginx/strange test.txt"

Copy link
Member

@craciunoiuc craciunoiuc left a comment

Choose a reason for hiding this comment

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

The code looks okay, waiting on your response for the comments above

@MdSahil-oss
Copy link
Contributor Author

Hey @MdSahil-oss found 1 small bug but otherwise it seems to work:

test.txt <-- works (ignored)

# test1.txt <-- works (not ignored)

nginx/test3.txt    #       test2.txt  <-- works (both ignored, basically # was ignored)

test.txt <-- works (nothing changed)

# test.txt <-- works (nothing changed)

./nginx <-- works (ignored)

./nginx/* <-- works (prints warning)

.///////////////////nginx <-- fails (multiple slashes are usually ignored)
./nginx/../test1.txt <-- fails (I think .. is not understood)
././././././././././././test1.txt <-- fails (I think . is not understood)

Nice catch @craciunoiuc, I will work on your suggestions

@MdSahil-oss
Copy link
Contributor Author

MdSahil-oss commented Jan 16, 2024

One more thing: does it support using "" or '' characters? I was thinking about the case of "./nginx/strange test.txt"

Don't think so, I will update this PR for these characters.

Copy link
Member

@craciunoiuc craciunoiuc left a comment

Choose a reason for hiding this comment

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

Hey @MdSahil-oss, some more comments from my side

initrd/kraftignore.go Outdated Show resolved Hide resolved
initrd/kraftignore.go Outdated Show resolved Hide resolved
initrd/kraftignore.go Outdated Show resolved Hide resolved
continue
}

item = strings.TrimPrefix(item, "..")
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I don't think we should trim this, but rather what I would do would be:

  1. Find the full path of the initrd directory
  2. Append the file path to the initrd directory
  3. Check if it evaluates to a valid file, if yes, trim 1. from it and use that given path, if not, ignore

This goes for the whole file parsing logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the logic to work in your recommended way, Please review again.

But still removing ../ if item (a path mentioned in .kraftignore) contains ../ as prefix because path inside rootfs which starts ../xyz refer to the file/dir outside of rootfs dir.

I hope this make sense to you otherwise ping me again ;)

@MdSahil-oss MdSahil-oss force-pushed the kraftignore branch 2 times, most recently from bda9ebc to af27038 Compare January 20, 2024 12:34
Copy link
Member

@nderjung nderjung left a comment

Choose a reason for hiding this comment

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

Hi @MdSahil-oss! Thanks so much for this contribution, I think it's a valuable addition to the usability of kraft! I've left you some feedback as in-line comments below. Overall, the implementation looks good.

initrd/kraftignore.go Outdated Show resolved Hide resolved
initrd/options.go Outdated Show resolved Hide resolved
initrd/options.go Outdated Show resolved Hide resolved
initrd/directory.go Outdated Show resolved Hide resolved
initrd/kraftignore.go Outdated Show resolved Hide resolved
@MdSahil-oss MdSahil-oss force-pushed the kraftignore branch 2 times, most recently from 9077346 to 16e83d8 Compare January 22, 2024 11:49
initrd/directory.go Outdated Show resolved Hide resolved
Comment on lines +23 to +29
type IgnoringFileType string

const (
Exist = IgnoringFileType("Exist")
NotExist = IgnoringFileType("NotExist")
SkipDir = IgnoringFileType("SkipDir")
)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, we can declare these as errors:

var (
  ErrFileExists    = fmt.Errorf("file exists")
  ErrFileNotExists = fmt.Errorf("filt does not exist")
  ErrIgnoreDir     = fmt.Errorf("ignored directory")
)

Copy link
Member

Choose a reason for hiding this comment

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

You can then do comparisons via:

if errors.Is(err, ErrFileExists)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surely, It can be done. But kraftiignore.go not uses these vars as errors, These vars are being used as notifiers for filepath.WalkDir() function in directory.go.

Please have an eye on filepath.WalkDir() function then lemme know if you really want me to change const vars as error vars :)

Signed-off-by: Md Sahil <mohdssahil1@gmail.com>
Signed-off-by: MdSahil-oss <mohdssahil1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🧊 Icebox
Development

Successfully merging this pull request may close these issues.

Introduce .kraftignore
3 participants