search

dnephin / dobi

A build automation tool for Docker applications
https://dnephin.github.io/dobi/
Apache License 2.0
309 stars 36 forks source link

Fix sources/artifacts bug introduced with #184 #209

Closed siredmar closed 3 years ago

siredmar commented 3 years ago

This fixes issue #200. The file handling introduced with #184 relies on absolute paths.

It checks if the path is relative. If so, prepent the cwd to the path. e.g. 

job=build:
    use: builder
    sources: 
      - source/*.cpp
...

In the above example the paths in sources are relative. They are located within the project where the dobi.yaml is located. So internally it creates <cwd>/source/*.cpp.

siredmar commented 3 years ago

@dnephin would you mind making a release 0.15.0 once this is merge? Thx!

siredmar commented 3 years ago

@dnephin this is just a ping

siredmar commented 3 years ago

@dnephin this is another ping. Can you merge this please, if is good enough?

dnephin commented 3 years ago

Thank you for working on this fix! Sorry it has taken me some time to have a look. I started looking and I found a few more problems. I made some changes here: https://github.com/dnephin/dobi/commit/2aa660834b35f8300dba1f30e020bf96ef96b39b

I noticed that all of the callers to this function should be passing Root by getting it from the ExecuteContext, but there are a few places that do not.

I think instead of making everything an absolute path , then later making it a relative path, it should keep relative paths as they are, and only use filepath.Rel when it receives an absolute path.

I'll see if I can get that working today, but it might still need some work.

dnephin commented 3 years ago

I rebased your changes on the latest master and added another commit. Does this work to solve the bug you hit? The tests are passing, but I made some changes to them, and I'm not sure if they still accurately reproduce the problem.

The change I made was to require that LastModifiedSearch.Root always be set to an absolute path, and I documented that on the field. All callers should now use an absolute path, so filepath.Rel should work.

dnephin commented 3 years ago

hmm, it looks like this change to make the context relative to the working directory is not correct. The tests are failing.

siredmar commented 3 years ago

@dnephin without investigated the issue the failing circle ci tests might fail because the paths might put <absolute>/<relative> together. This is the image that fails: Example

image=builder:
    image: dobi-dev
    context: dockerfiles/
    dockerfile: Dockerfile.build

And the error stating <absolute>/<relative> for path:

[ERROR] failed to execute task "builder:build": internal error: stat /root/project/dockerfiles/dockerfiles: no such file or directory
siredmar commented 3 years ago

@dnephin i was right. For the stale check in the image build the root path was false set. The task configuration brings the correct context, therefore no need to append the context to the workingDir. I have no idea what other possible errors are hidden after those changes in the other files. However, the unit tests are succeeding now.

siredmar commented 3 years ago

@dnephin i can confirm with my manual tests that the current state of this branch solves my issues. You might want to merge this now.

siredmar commented 3 years ago

@dnephin Can this get merged now?

siredmar commented 3 years ago

@dnephin Is this project dead now?

siredmar commented 3 years ago

@dnephin I don't want to be a pain, but in the meantime I need a fixed release with an even older feature that hasn't been released yet. So would you please merge this and create a new release 0.15.0?

dnephin commented 3 years ago

https://github.com/dnephin/dobi/releases/tag/v0.15.0