dalec icon indicating copy to clipboard operation
dalec copied to clipboard

Implement FS.ReadDirFs to properly determine type of context source

Open DannyBrito opened this issue 1 year ago • 8 comments

What this PR does / why we need it:

The issue is that when trying to use a context source as a patch, it is currently treated as directory, so the file processed to a tar file, and when trying to apply the patch is not found under the sources as it is not handled.

This doesn't fix the context issue to be able to handle different for a file or directory.

These simple aims to allow context source to be used a patch.

I would appreciate so guidance/feedback on how to properly go about this issue.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged): Fixes #

Special notes for your reviewer:

Fixes #188

DannyBrito avatar Mar 25 '24 21:03 DannyBrito

Thanks to @adamperlin, helping to implement ReadDirFs for Sources, we play a bit with the concept of using that to Figure out if a source should be treated as dir or not. However, it seems like this is way more complex than expected. So, I would like to gather feedback of how these should be handled.

While playing with how this should be handle is a bit tricky due to the flexibility to allow a source to include, exclude, and define a path of the source: Here are some scenarios that I imagine we could possible encounter: PATH has greater presendece over includes and possible excludes? Ni -> No included, Ai -> Any included Ne -> No excluded, Ae -> Any excluded

  • IF path isn't empty and points to file [Ni,Ne]-> it should be a file
  • IF path isn't empty and points to dir [Ni,Ne] -> it should be a dir
  • IF path isn't empty and points to file [Ai,Ae] -> it should be a file it should have precedence? seems that if they include block is present it requires an exact match to the path to not break.
  • IF path isn't empty and points to dir [Single Include to file, Ne] -> it should be a file if path is parent path of the include file, if include file is nested, it probably should be a dir.
  • IF path isn't empty and points to dir [Single Include to dir, Ne] -> it should be a dir
  • IF path isn't empty and points to dir [Multiple Includes, NE] -> it should be a dir
  • IF path isn't empty and points to dir [Ni,Excludes all dirs] only one file remains in path as parent directory it should be a file, if remained file is in a nested path it should be a dir?
  • IF path is empty [Multiple includes, Ne] it should be a dir
  • IF path is empty [Single include not nested path, Ne] it should be a file
  • IF path is empty [Single include nested path, Ne] it should be a dir

Should path: ./ field work and include everything, this might be broken.

Maybe, I am overthinking this, please any feedback would be great!

DannyBrito avatar Apr 04 '24 18:04 DannyBrito

Other question is as we are using the FS implementation this needs to have a state. However, there is an issue we encounter with it as calling AsState for the source will also be calling to get the sources and these sources are getting filter accordingly. This affects to the original paths. Example if source has path ./to/the/source.txt after filter and function the source would present at state with the source.txt file but this would be found at the / root level. So not encounter this problem of no longer valid paths, it was to invoke the IsDir right after getting sources and just before the filter within the AsState function and change returns to include a bool if isDIr. Would that be an ok approach? or any other suggestion would be apricated

DannyBrito avatar Apr 04 '24 18:04 DannyBrito

So we are looking at this 1 source at a time. If there are nested sources, we still only care about the source we are currently working with.

With filter/includes, this is implying a directory with one or more things in it. I think we should treat that as a directory.

I think all we really need to address is if the Path is pointing at a file or a directory.

cpuguy83 avatar Apr 04 '24 18:04 cpuguy83

I think the example we were thinking of was something like this (wanting to include a single patch file from build context). And the question is, what would be the ideal way to do this, and have the source be treated as just a file?

 mypatch:
    path: ./patches/mypatch.patch
    includes:
       - ./patches/mypatch.patch
    context:
      name: context

In this case, we are using includes and excludes, but we really only want to include a single file. So I think regardless of whether there are includes or excludes, if the path points to a file in the original source state, we should treat it as a file?

Slightly related (thought it doesn't solve the issue as a whole), for the patching case we could modify the patching code to always apply patches as a directory to support #185 with one or more files, and ensure any patches are pre-nested if they aren't already.

adamperlin avatar Apr 04 '24 20:04 adamperlin

Correct, whatever path is pointing to should be the source of truth there. The include/exclude is just optimization.

cpuguy83 avatar Apr 04 '24 21:04 cpuguy83

The CI is failing in two sections:

  1. The Unit Test is failing on test: https://github.com/Azure/dalec/blob/main/source_test.go#L327 due to sOpts not really having available the GetFS function - that is used to determine if sources is a dir/file. The Issue here is that we cannot directly call to GetFS because worked implementation relies. on having the buildkit client, which in this case for the tests is not available. What would be the best to go about this issue?
  2. The integration test: this is related to the windows tests and fails when gathering the sources and copy these to the windows base image, it is not really clear to me why this is failing, the local logs point out the base image path is not present https://github.com/Azure/dalec/blob/main/frontend/windows/handle_container.go#L60, for windowsSystemDir, when the copy is taking place. Note* This test is failing for me locally as well against main branch but not in the CI against main. I would appreciate some assistance debugging this.

DannyBrito avatar Apr 16 '24 18:04 DannyBrito

The Issue here is that we cannot directly call to GetFS

Can we provide a custom implementation for the test? This is what we do for some other tests (that use other client functions).

cpuguy83 avatar Apr 16 '24 22:04 cpuguy83

The current State on how we determine if a context source is a dir or file. Is after getting the source and pass it down the filter, we will check the FS on / in the result state. If we have a single entry in the / level and is a file, then it will be renamed to match source name and treated as a file. Otherwise is treated as a dir and not renamed

DannyBrito avatar Apr 18 '24 18:04 DannyBrito