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

New JavaLayerConfiguration API for more flexibility #1049

Merged
merged 8 commits into from
Sep 27, 2018

Conversation

chanseokoh
Copy link
Member

Fixes #1007.

See if this sounds reasonable.

I will see if I can further simplify GradleLayerConfigurations and MavenLayerConfigurations with the new API.

* sourceFile}
* @return this
*/
public Builder addDependencyFileRecursive(Path sourceFile, AbsoluteUnixPath pathInContainer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it okay if all files are added recursively? (aka are the non-rescursive add...File methods necessary?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to be in line with LayerConfiguration having both recursive and non-recursive methods, but now I think it is possible to cover all cases only with recursive adds. Then perhaps I can rename add...FileRecursive() to just add...File.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Member Author

Choose a reason for hiding this comment

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

Now I remember. The non-recursive case is useful when the caller is walking a directory, enumerating all files and directories and adding each path.

Copy link
Contributor

@coollog coollog left a comment

Choose a reason for hiding this comment

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

LGTM I think we should add a CHANGELOG note that the Docker context layout has changed.

@chanseokoh
Copy link
Member Author

Added CHANGELOG entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants