Skip to content
This repository has been archived by the owner on Oct 25, 2023. It is now read-only.

Extract common buildpack code #139

Merged
merged 1 commit into from
Oct 5, 2022
Merged

Extract common buildpack code #139

merged 1 commit into from
Oct 5, 2022

Conversation

menehune23
Copy link
Contributor

@menehune23 menehune23 commented Oct 5, 2022 •
edited
Loading

Introduces a common go module from which to share code that is common among buildpacks

  • Moves python/java.FuncYaml (and tests) to common config.FuncYaml
  • Moves python.CommandRunner to common command.Runner

TODO:

@menehune23 menehune23 requested a review from a team October 5, 2022 00:59
@menehune23 menehune23 force-pushed the am/common-code branch 3 times, most recently from 734b34e to 2f87db0 Compare October 5, 2022 01:09
@menehune23 menehune23 force-pushed the am/python-layer-contrib-tests branch from d81f39c to b82230f Compare October 5, 2022 01:11
@menehune23 menehune23 force-pushed the am/common-code branch 2 times, most recently from ec8d50a to 2eedbe4 Compare October 5, 2022 01:23
@menehune23
Copy link
Contributor Author

menehune23 commented Oct 5, 2022 •
edited
Loading

@andrew-su Is there something I'm missing with the dependency review? I see similar go.sum entries for other modules on the main branch (and in other PRs, like #137) which don't fail.

@andrew-su
Copy link
Contributor

I wonder if this comes from it being a new go.mod and go.sum.

@andrew-su
Copy link
Contributor

From https://github.com/vmware-tanzu/function-buildpacks-for-knative/actions/runs/3184247436 (Failed dependency review run for #137) it looks like it did complain about it in the beginning, but it doesn't care about old existing problem after you pushed some more changes.

@menehune23 menehune23 force-pushed the am/python-layer-contrib-tests branch from b82230f to db88bc7 Compare October 5, 2022 17:49
@menehune23 menehune23 force-pushed the am/common-code branch 2 times, most recently from e991e59 to c02265e Compare October 5, 2022 18:18
@menehune23
Copy link
Contributor Author

menehune23 commented Oct 5, 2022 •
edited
Loading

@andrew-su Using go mod graph I was able to trace at least one of the offending deps back to kn-plugin-func. That project has progressed quite far since our original inclusion, and upgrading it could result in larger refactors. Are you comfortable approving this PR without updating the deps in the new common project, and doing this in a later PR? The versions used are identical to those used in our other projects, so it's not materially introducing any new vulnerabilities.

Base automatically changed from am/python-layer-contrib-tests to main October 5, 2022 18:37
@andrew-su
Copy link
Contributor

There's an issue: #55 for updating kn-plugin-func to the latest version. IIRC, they updated their code for the plugin and made it unusable as a library. Maybe things have changed and we should probably revisit that. I'd very much like to not be on such an old version.

menehune23
menehune23 commented Oct 5, 2022
View reviewed changes
buildpacks/java/tests/setup.go
)

func NewLogger() bard.Logger {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this file moved to the common dir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's left is java-specific

menehune23
menehune23 commented Oct 5, 2022
View reviewed changes
buildpacks/python/tests/setup.go
@@ -50,126 +46,7 @@ func (f HTTPFunction) Generate(path, funcTemplate string) error {
return templ.Execute(file, f)
}

func NewLogger() bard.Logger {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this file moved to the common dir. What's left is python-specific

andrew-su
andrew-su approved these changes Oct 5, 2022
View reviewed changes
Copy link
Contributor

@andrew-su andrew-su left a comment

Choose a reason for hiding this comment

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

Everything else looks 👍

buildpacks/Makefile Outdated Show resolved Hide resolved
@menehune23 menehune23 merged commit bac7bd5 into main Oct 5, 2022
@menehune23 menehune23 deleted the am/common-code branch October 5, 2022 21:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@andrew-su andrew-su andrew-su 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.
2 participants
@menehune23 @andrew-su