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

Adding support to handle multiple artifacts #93

Closed
wants to merge 3 commits into from
Closed

Adding support to handle multiple artifacts #93

wants to merge 3 commits into from

Conversation

jjbustamante
Copy link
Contributor

@jjbustamante jjbustamante commented Dec 9, 2021

Summary

The idea behind this changes is extend the behavior of the ArtifactResolver to handle more than 1 artifact after a build is executed. Currently, we are expecting just 1 file (zip equivalent format), this file is zipped, copy to the /layer and then restore in /workspace.

The Pattern() regular expression is evaluated and the result is analize following these rules

  • It matches a single file (current behavior)
  • It matches multiple files (multi-file behavior)
  • It matches a folder (take everything in the folder, then multi-file behavior)
  • It matches multiple folders (take everything in every folder, then multi-file behavior)

Use Cases

The initial particular use case for this feature is to help the native-image creation of an application from source code that is not exploded, for example, in case of Quarkus application a folder with all the dependencies require build the native-image can be created using the variable: -Dquarkus.package.type=native-sources for example in Maven, the output of this build is a folder in my build directory with a structure similar to this:

target/native-sources
├── target/native-sources
├── lib
└── native-image.args

In this particular case, this PR can handle the result of this build an copy all the files inside the target/native-sources folder

This PR Fixes paketo-buildpacks/maven/issues/76

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@jjbustamante jjbustamante requested a review from a team December 9, 2021 12:22
@dmikusa dmikusa added semver:minor A change requiring a minor version bump type:enhancement A general enhancement labels Jan 20, 2022
@dmikusa
Copy link
Contributor

dmikusa commented Jan 20, 2022

@jjbustamante I did a review of this and dug around through the code. The gist of what you have looks good to me.

I pushed this branch with some ideas to reduce some of the code, some additional tests I wanted to see, and also a change to the behavior of how we store folders under the layer.

Can you take a look and let me know what you think? In particular, about the last bit. That will change the structure of what is restored into the /workspace, so I want to make sure that will be OK for your usecase.

If that all looks good, I can merge that branch into main.

Thanks!

@jjbustamante
Copy link
Contributor Author

jjbustamante commented Jan 20, 2022

@jjbustamante I did a review of this and dug around through the code. The gist of what you have looks good to me.

I pushed this branch with some ideas to reduce some of the code, some additional tests I wanted to see, and also a change to the behavior of how we store folders under the layer.

Can you take a look and let me know what you think? In particular, about the last bit. That will change the structure of what is restored into the /workspace, so I want to make sure that will be OK for your usecase.

If that all looks good, I can merge that branch into main.

Thanks!

Hi @dmikusa-pivotal thanks for the review and the feedback!!

Let me take a look at it, but more importantly, I will run the use case I was testing which is building a Quarkus sample app from the started guide using Pack + Buildpack! I will let you know if everything is still working with this branch

@jjbustamante
Copy link
Contributor Author

jjbustamante commented Jan 20, 2022

Hi @dmikusa-pivotal.

I tried to test this branch on my local with the Quarkus project I was using before, but when I tried to build the Maven Buildpack to point to this branch I am getting this

Screen Shot 2022-01-20 at 4 50 14 PM

The only thing I did was to add this line into the go.mod

replace github.com/buildpacks/libcnb => ../libbs

Any idea?

@dmikusa
Copy link
Contributor

dmikusa commented Jan 20, 2022

I have not had good luck with relative paths in replace statements for go.mod files. Perhaps try a full path? I'm not sure if relative is supposed to work or not. It failed once and now I just always use full paths.

@jjbustamante
Copy link
Contributor Author

Sorry, my mistake @dmikusa-pivotal. I was replacing libcnb instead of libbs :$

@jjbustamante
Copy link
Contributor Author

@dmikusa-pivotal

I tested the branch and it worked, I managed to build the native image changing the path where the artifacts are copied.

Screen Shot 2022-01-20 at 6 29 16 PM
Screen Shot 2022-01-20 at 6 29 25 PM

I need to modify the native-image PR to handle this (for testing I hardcoded to run the validation), I will also check some comments you left there. From my side, I am happy with this new branch!

@dmikusa
Copy link
Contributor

dmikusa commented Jan 26, 2022

OK, I submitted & merged #100. Closing this.

@dmikusa dmikusa closed this Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] support multiple artifacts of different types.
2 participants