-
Notifications
You must be signed in to change notification settings - Fork 65
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
base: staging
Are you sure you want to change the base?
Conversation
1603cdb
to
2e0f395
Compare
47907c8
to
32e420e
Compare
.kraftignore
support (WIP).kraftignore
(WIP)
d6461e2
to
9f7f0ce
Compare
.kraftignore
(WIP).kraftignore
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.
Some initial comments from my side, did not try out the code yet
9f7f0ce
to
791a7ad
Compare
791a7ad
to
9bb1054
Compare
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 |
Hey @MdSahil-oss found 1 small bug but otherwise it seems to work:
|
One more thing: does it support using |
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.
The code looks okay, waiting on your response for the comments above
Nice catch @craciunoiuc, I will work on your suggestions |
Don't think so, I will update this PR for these characters. |
9bb1054
to
3d6884c
Compare
3d6884c
to
2da6c81
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.
Hey @MdSahil-oss, some more comments from my side
initrd/kraftignore.go
Outdated
continue | ||
} | ||
|
||
item = strings.TrimPrefix(item, "..") |
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.
Same here, I don't think we should trim this, but rather what I would do would be:
- Find the full path of the initrd directory
- Append the file path to the initrd directory
- 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
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.
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 ;)
bda9ebc
to
af27038
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.
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.
9077346
to
16e83d8
Compare
type IgnoringFileType string | ||
|
||
const ( | ||
Exist = IgnoringFileType("Exist") | ||
NotExist = IgnoringFileType("NotExist") | ||
SkipDir = IgnoringFileType("SkipDir") | ||
) |
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.
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")
)
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.
You can then do comparisons via:
if errors.Is(err, ErrFileExists)
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.
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 <[email protected]>
Signed-off-by: MdSahil-oss <[email protected]>
a05cf67
to
8387c83
Compare
what is the state on this @MdSahil-oss @nderjung I think it was almost ready to go out? |
Prerequisite checklist
make fmt
on your commit series before opening this PR;Description of changes
This PR fixes #135
Specifying contents (file or directory paths) in
.kraftignore
file in the working directory forcesKraftkit
to ignore matching children files or directories sharing from the sharing directory asrootfs
.For now,
.kraftignore
only work whenrootfs
is specified for a directory path.Rules for
.kraftignore
:Line starts with a
#
will be ignored byKraftkit
.Every blank line will be ignored by
Kraftkit
.Directory or File path that contains
glob-patterns
(characters like*
,!
,[
and?
) will be ignored byKraftkit
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.
One needs to specify the entire relative path (relative path to the sharing directory e.g. if
rootfs=share/
andshare
directory containssample.txt
andsample/
, So relative path in.kraftignore
file can be defined as/sample.txt
,sample.txt
and./sample.txt
forshare/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.All the specified paths in
.kraftignore
will be relative to the root of sharing directory, means ifshare/
directory containssample.txt
andsample/
, and set asrootfs
to kraftkit then to ignore/share/sample.txt
onlysample.txt
needs to be defined inkraftignore
.Note: All the paths can start with
./
or/
(Since prefix/
and./
with paths simply ignored by Kraftkit).