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

Make compatible with pack 0.3.0 #154

Merged
merged 1 commit into from
Aug 7, 2019
Merged

Make compatible with pack 0.3.0 #154

merged 1 commit into from
Aug 7, 2019

Conversation

ameyer-pivotal
Copy link
Contributor

@ameyer-pivotal ameyer-pivotal commented Jul 23, 2019

Signed-off-by: Andrew Meyer ameyer@pivotal.io

Added compat package that's pretty self-contained and can be easily removed when we no longer want this functionality.

@ameyer-pivotal ameyer-pivotal requested a review from sclevine July 23, 2019 18:16
@ameyer-pivotal ameyer-pivotal force-pushed the more-dist-spec-updates branch from c3fe218 to 160af39 Compare July 23, 2019 18:22
@sclevine sclevine force-pushed the dist-spec-and-build-plan branch 7 times, most recently from 6e07d44 to 58b8064 Compare July 26, 2019 06:18
@ameyer-pivotal ameyer-pivotal force-pushed the more-dist-spec-updates branch 2 times, most recently from ea67f9e to 9b65887 Compare August 6, 2019 15:05
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.

Approved with some minor suggestions.

@@ -43,9 +44,16 @@ func main() {
}

func detect() error {
order, err := lifecycle.ReadOrder(orderPath)
order, err := compat.ReadOrder(orderPath, buildpacksDir)
Copy link
Member

Choose a reason for hiding this comment

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

nit: should we read the new order format first and then fall back to the old format if the length is 0? It seems like that would be more in keeping with an "ident the error handling" philosophy

return order, nil
}

type buildpackTOML struct {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a compat definition of buildpackToml and buildpack, it looks like the differences between the definitions and the ones in lifecycle.go and these are purely additive

Copy link
Member

Choose a reason for hiding this comment

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

I am gonna assume this is for the sake of isolating the compat work in a package and avoiding import cycles.

}

if len(matchVersions) > 1 {
return "", errors.Errorf("too many buildpacks with matching ID '%s'", bpRef.ID)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should something specific about "latest" in this error message. Given that there is no requirement that only one version of a buildpack can exist on a builder and in contexts like --buildpack <id> it might not be obvious what the issue is.

@sclevine sclevine force-pushed the dist-spec-and-build-plan branch from 58b8064 to a13fe0a Compare August 7, 2019 17:28
@sclevine sclevine changed the base branch from dist-spec-and-build-plan to master August 7, 2019 22:25
Signed-off-by: Andrew Meyer <ameyer@pivotal.io>
Signed-off-by: Javier Romero <jromero@pivotal.io>
@sclevine sclevine force-pushed the more-dist-spec-updates branch from 9b65887 to ccb05b2 Compare August 7, 2019 22:32
@sclevine sclevine merged commit 12fb4c6 into master Aug 7, 2019
@ekcasey ekcasey deleted the more-dist-spec-updates branch November 25, 2019 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants