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

Process project.toml for app source metadata #1231

Merged
merged 25 commits into from
Aug 10, 2021

Conversation

Haimantika
Copy link
Contributor

@Haimantika Haimantika commented Jul 15, 2021

Summary

  1. Parsed the file (made changes to project.go)
  2. Wrote the project-metdata. toml file (changes to be found in container_ops.go, container_ops_test.go, lifecycle_execution_test.go and lifecycle_execution.go)

Resolves #1138

Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
@Haimantika Haimantika requested a review from a team as a code owner July 15, 2021 06:04
@github-actions github-actions bot added this to the 0.20.0 milestone Jul 15, 2021
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Jul 15, 2021
Copy link
Contributor

@aemengo aemengo left a comment

Choose a reason for hiding this comment

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

Thanks for helping us out with this 🙏🏾

Let's please not have these types of files as part of the PR (internal/registry/registry468683854).

I made this issue about removing these artifacts after the tests run: #1228

Signed-off-by: haimantika mitra <[email protected]>
…he needs of the issue

Signed-off-by: haimantika mitra <[email protected]>
…go.mod and go.sum files

Signed-off-by: haimantika mitra <[email protected]>
@github-actions github-actions bot added the type/chore Issue that requests non-user facing changes. label Jul 17, 2021
jromero and others added 3 commits July 17, 2021 20:24
Signed-off-by: Javier Romero <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
@jromero jromero modified the milestones: 0.20.0, 0.21.0 Jul 21, 2021
Signed-off-by: haimantika mitra <[email protected]>
@github-actions github-actions bot modified the milestones: 0.21.0, 0.20.0 Jul 24, 2021
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
Comment on lines 47 to 50
func (d *Descriptor) ProjectMetadata() platform.ProjectMetadata {
return platform.ProjectMetadata{}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

As much as I like it, these lines won't be needed in your final implementation, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, we can do without. Maybe I can comment this out,

Copy link
Member

Choose a reason for hiding this comment

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

If something isn't necessary, we delete it, not comment out. 😁

Copy link
Member

@jromero jromero left a comment

Choose a reason for hiding this comment

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

Made a few change requests but overall the feature looks great!

Also, it looks like there's a minor conflict with build.go. We'll want to resolve that so we can run all the tests on GitHub.

acceptance/acceptance_test.go Outdated Show resolved Hide resolved
@@ -901,6 +904,9 @@ func testAcceptance(
t.Log("sets the run image metadata")
assertImage.HasLabelWithData(repoName, "io.buildpacks.lifecycle.metadata", fmt.Sprintf(`"stack":{"runImage":{"image":"%s","mirrors":["%s"]}}}`, runImage, runImageMirror))

t.Log("sets the source metadata")
assertImage.HasLabelWithData(repoName, "io.buildpacks.project.metadata", fmt.Sprintf(`{"project-metadata":{"source":{"type":"%s","version":{"declared":"%s"},"metadata":{"url":"%s"}}}`, Type, version, metadata))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assertImage.HasLabelWithData(repoName, "io.buildpacks.project.metadata", fmt.Sprintf(`{"project-metadata":{"source":{"type":"%s","version":{"declared":"%s"},"metadata":{"url":"%s"}}}`, Type, version, metadata))
assertImage.HasLabelWithData(repoName, "io.buildpacks.project.metadata", `{"project-metadata":{"source":{"type":"project","version":{"declared":"1.0.2"},"metadata":{"url":"https://github.com/buildpacks/pack"}}}`)

@@ -0,0 +1,3 @@
[project]
version = "1.0.2"
source-url = "https://github.com/buildpacks/pack"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
source-url = "https://github.com/buildpacks/pack"
source-url = "https://github.com/buildpacks/pack"

I hate this but we should have a new line character at the end of each file. See https://unix.stackexchange.com/q/18743

@@ -19,6 +19,8 @@ import (
"github.com/pkg/errors"
ignore "github.com/sabhiram/go-gitignore"

"github.com/buildpacks/lifecycle/platform"
Copy link
Member

Choose a reason for hiding this comment

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

Import Groups

Imports should be grouped into the following:

  1. Standard library
  2. External libraries
  3. Local packages

Comment on lines 172 to 189
// func createReader(src, dst string, uid, gid int, includeRoot bool, fileFilter func(string) bool) (io.ReadCloser, error) {
// fi, err := os.Stat(src)
// if err != nil {
// return nil, err
// }

// if fi.IsDir() {
// var mode int64 = -1
// if runtime.GOOS == "windows" {
// mode = 0777
// }

// return archive.ReadDirAsTar(src, dst, uid, gid, mode, false, includeRoot, fileFilter), nil
// }

// return archive.ReadZipAsTar(src, dst, uid, gid, -1, false, fileFilter), nil
// }

Copy link
Member

Choose a reason for hiding this comment

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

We don't want to commit unused code

Suggested change
// func createReader(src, dst string, uid, gid int, includeRoot bool, fileFilter func(string) bool) (io.ReadCloser, error) {
// fi, err := os.Stat(src)
// if err != nil {
// return nil, err
// }
// if fi.IsDir() {
// var mode int64 = -1
// if runtime.GOOS == "windows" {
// mode = 0777
// }
// return archive.ReadDirAsTar(src, dst, uid, gid, mode, false, includeRoot, fileFilter), nil
// }
// return archive.ReadZipAsTar(src, dst, uid, gid, -1, false, fileFilter), nil
// }

image

@@ -15,6 +15,8 @@ import (
"github.com/docker/docker/client"
"github.com/pkg/errors"

"github.com/buildpacks/lifecycle/platform"
Copy link
Member

Choose a reason for hiding this comment

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

See "Import Groups"

@@ -20,6 +20,8 @@ import (
"github.com/sclevine/spec"
"github.com/sclevine/spec/report"

"github.com/buildpacks/lifecycle/platform"
Copy link
Member

Choose a reason for hiding this comment

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

See "Import Groups"

project.toml Outdated
@@ -0,0 +1,3 @@
[project]
version = "1.0.2"
source-url = "https://github.com/buildpacks/pack"
Copy link
Member

Choose a reason for hiding this comment

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

nit: "New line" character missing at end of file

@@ -5,8 +5,10 @@ import (
"path/filepath"

"github.com/BurntSushi/toml"
//"github.com/docker/docker/api/types/versions"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//"github.com/docker/docker/api/types/versions"

Comment on lines 47 to 50
func (d *Descriptor) ProjectMetadata() platform.ProjectMetadata {
return platform.ProjectMetadata{}
}

Copy link
Member

Choose a reason for hiding this comment

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

If something isn't necessary, we delete it, not comment out. 😁

Haimantika and others added 2 commits July 30, 2021 11:58
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
Signed-off-by: haimantika mitra <[email protected]>
@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #1231 (946e2e6) into main (66a4f32) will decrease coverage by 1.11%.
The diff coverage is 76.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1231      +/-   ##
==========================================
- Coverage   80.99%   79.88%   -1.10%     
==========================================
  Files         140      140              
  Lines        8534     8568      +34     
==========================================
- Hits         6911     6844      -67     
- Misses       1184     1284     +100     
- Partials      439      440       +1     
Flag Coverage Δ
os_linux 80.64% <80.00%> (-0.02%) ⬇️
os_macos 78.00% <54.29%> (-0.16%) ⬇️
os_windows 79.80% <76.20%> (-1.10%) ⬇️
unit 80.64% <80.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@jromero jromero changed the title Parsed file and added project_metadata.toml file- Issue #1138 Process project.toml for app source metadata Aug 9, 2021
@jromero jromero removed the type/chore Issue that requests non-user facing changes. label Aug 9, 2021
@jromero jromero modified the milestones: 0.20.0, 0.21.0 Aug 9, 2021
@jromero jromero mentioned this pull request Aug 9, 2021
@jromero jromero merged commit 946e2e6 into buildpacks:main Aug 10, 2021
@jromero jromero added the experimental Issue or PR refers to an experimental feature. label Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Issue or PR refers to an experimental feature. type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide project source metadata from project.toml
3 participants