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

Change defaults that are depending on the working dir to be under <layers> #454

Merged
merged 6 commits into from
Nov 13, 2020

Conversation

yaelharel
Copy link
Contributor

@yaelharel yaelharel commented Oct 29, 2020

Notes:

  • This commit introduces platfromAPI 0.5 as a supported API.
  • The changes apply only for platformAPI >= 0.5.
  • The path of the relevant files is being changed as part of Args after parsing the flags.
  • Allow the user to set the layers flag in the detector.

Fixes #393

…yers>

Notes:
* This commit introduces platfromAPI 0.5 as a supported API.
* The changes apply only for platformAPI >= 0.5.
* The path of the relevant files is being changed as part of Args after parsing the flags.
* Allow the user to set the layers flag in the detector.

Signed-off-by: Yael Harel <[email protected]>
@yaelharel yaelharel requested a review from a team as a code owner October 29, 2020 18:44
cmd/command.go Outdated Show resolved Hide resolved
cmd/command.go Outdated Show resolved Hide resolved
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.

Although we don't think we need to acceptance test every affected flag I think having at least one acceptance test that uses a default relative to layers dir would be good.

This analyzer test seems like a good candidate https://github.com/buildpacks/lifecycle/blob/main/acceptance/analyzer_test.go#L162-L178

It should pass now even if you remove the -analyzed flag. If we alter that test we should add another test that sets -analyzed but not -layers, to ensure both cases work.

cmd/command.go Outdated Show resolved Hide resolved
cmd/flags.go Outdated Show resolved Hide resolved
cmd/flags.go Outdated Show resolved Hide resolved
cmd/command.go Outdated Show resolved Hide resolved
cmd/command.go Outdated Show resolved Hide resolved
cmd/flags.go Outdated Show resolved Hide resolved
cmd/lifecycle/analyzer.go Outdated Show resolved Hide resolved
cmd/lifecycle/builder.go Outdated Show resolved Hide resolved
cmd/lifecycle/detector.go Outdated Show resolved Hide resolved
cmd/lifecycle/exporter.go Outdated Show resolved Hide resolved
* Need to fix a new test in analyzer_test.go

Signed-off-by: Yael Harel <[email protected]>
acceptance/analyzer_test.go Outdated Show resolved Hide resolved
@yaelharel
Copy link
Contributor Author

@ekcasey, I pushed a commit with some fixes following your feedback.
I added two new tests to analyzer_test.go and one of them is failing due to

ERROR: write analyzed.toml: open /other-layers/other-analyzed.toml: permission denied

Please ignore it for now and I'll push a fix as soon as possible.

cmd/command.go Outdated Show resolved Hide resolved
cmd/flags.go Outdated Show resolved Hide resolved
acceptance/testdata/analyzer/analyze-image/Dockerfile Outdated Show resolved Hide resolved
acceptance/analyzer_test.go Outdated Show resolved Hide resolved
acceptance/analyzer_test.go Outdated Show resolved Hide resolved
cmd/flags.go Outdated Show resolved Hide resolved
cmd/flags.go Outdated Show resolved Hide resolved
Signed-off-by: Yael Harel <[email protected]>
@yaelharel yaelharel merged commit 79bde45 into main Nov 13, 2020
@yaelharel yaelharel deleted the 393-defaults-layers branch November 13, 2020 01:34
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.

Change defaults that depend on the working dir
2 participants