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.

Sign up for GitHub

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

Jump to bottom

Use version from toml file as BuildpackMap key #96

Merged
merged 4 commits into from
Apr 17, 2019
Merged

Use version from toml file as BuildpackMap key #96

merged 4 commits into from
Apr 17, 2019

Conversation

jkutner
Copy link
Member

@jkutner jkutner commented Mar 25, 2019

This facilitates the implementation of buildpacks/pack#113

@jkutner
Use version from toml file as BuildpackMap key
c75a50a 
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner jkutner force-pushed the fix-version-dirname branch from 4a8fb4c to c75a50a Compare March 25, 2019 21:13
@jkutner jkutner requested a review from ekcasey March 25, 2019 21:13
ekcasey
ekcasey reviewed Mar 26, 2019
View reviewed changes
Copy link
Member

@ekcasey ekcasey left a comment

Choose a reason for hiding this comment

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

I think we should add a test case to map_test.go with an escaped version directory to prevent regressions here (like we do with the escaped id).

@sclevine
Copy link
Member

I think this will break the latest symlinks.

@sclevine sclevine self-requested a review March 26, 2019 17:57
@jkutner
Copy link
Member Author

jkutner commented Apr 8, 2019 •
edited
Loading

@sclevine what do you think about treating latest as a special value (either in lifecycle or the spec). That is, a buildpack would not be allowed to define its version as "latest". That would allow me to work around this.

@sclevine
Copy link
Member

sclevine commented Apr 8, 2019

That restriction seems like a great idea to me. I don't think it makes the current proposed changes work with the latest symlink though.

@jkutner
Copy link
Member Author

jkutner commented Apr 8, 2019

@sclevine correct, i would still have to adjust this PR, but I wanted to float that idea before doing so

@jkutner jkutner mentioned this pull request Apr 9, 2019
Merged
@jkutner
Add support for 'latest' string as a reserved buildpack version
7a09ec3 
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner jkutner force-pushed the fix-version-dirname branch from b4de223 to 7a09ec3 Compare April 9, 2019 13:53
@jkutner
Copy link
Member Author

jkutner commented Apr 9, 2019

@ekcasey @sclevine this now incorporates the change proposed in buildpacks/spec#48 and is ready for re-review

@sclevine
Copy link
Member

sclevine commented Apr 9, 2019

Approved the changes, but agree with @ekcasey that test cases in map_test.go would be ideal.

jkutner added 2 commits April 9, 2019 15:44
@jkutner
Fix formatting in map.go
7cbe30d 
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner
Added a test for buildpack version 'latest' in NewBuildpackMap
1d26fa5 
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner
Copy link
Member Author

jkutner commented Apr 10, 2019

@sclevine @ekcasey added a test, but didn't use a symlink (i created a real dir with real files). I think that's probably a good idea?

@jkutner jkutner merged commit cabca3c into master Apr 17, 2019
@jkutner jkutner deleted the fix-version-dirname branch April 17, 2019 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekcasey ekcasey ekcasey left review comments

@sclevine sclevine sclevine approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jkutner @sclevine @ekcasey