Skip to content

feat!: support on cluster build from git repo with Tekton#743

Merged
knative-prow-robot merged 7 commits intoknative:mainfrom
zroubalik:git
Jan 12, 2022
Merged

feat!: support on cluster build from git repo with Tekton#743
knative-prow-robot merged 7 commits intoknative:mainfrom
zroubalik:git

Conversation

@zroubalik
Copy link
Copy Markdown
Contributor

@zroubalik zroubalik commented Jan 10, 2022

Signed-off-by: Zbynek Roubalik zroubali@redhat.com

Changes

  • 🎁 support on cluster build from git repo with Tekton
  • func deploy --build originally accepted true or false, now it accepts disabled, local or git values

/kind enhancement

on-cluster-git.mp4

Fixes: #748

Adds the ability to build a Function on the cluster using Tekton Pipelines. The build on the cluster is enabled by fetching Function source code from a remote Git repository.

@knative-prow-robot knative-prow-robot added kind/enhancement Feature additions or improvements to existing approved 🤖 PR has been approved by an approver from all required OWNERS files. size/XXL 🤖 PR changes 1000+ lines, ignoring generated files. labels Jan 10, 2022
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 10, 2022

Codecov Report

Merging #743 (c661d6f) into main (64ba17b) will decrease coverage by 0.27%.
The diff coverage is 35.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #743      +/-   ##
==========================================
- Coverage   41.52%   41.25%   -0.28%     
==========================================
  Files          42       44       +2     
  Lines        4101     4208     +107     
==========================================
+ Hits         1703     1736      +33     
- Misses       2145     2217      +72     
- Partials      253      255       +2     
Impacted Files Coverage Δ
knative/deployer.go 5.59% <0.00%> (ø)
cmd/deploy.go 12.59% <1.66%> (-3.30%) ⬇️
client.go 60.86% <42.30%> (-3.42%) ⬇️
function.go 55.00% <71.42%> (+0.86%) ⬆️
function_git.go 83.33% <83.33%> (ø)
function_buildtype.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64ba17b...c661d6f. Read the comment docs.

@zroubalik zroubalik force-pushed the git branch 6 times, most recently from b83a80e to 20ef9f3 Compare January 10, 2022 13:51
@zroubalik zroubalik changed the title feat: support on cluster build from git repo with Tekton feat!: support on cluster build from git repo with Tekton Jan 10, 2022
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@matejvasek
Copy link
Copy Markdown
Contributor

@zroubalik please fix the e2e tests (Error: specified build type "false" is not valid, allowed build types are "disabled", "local" or "git") and generated files ./hack/update-deps.sh.

Comment thread cmd/deploy.go Outdated
Comment thread pipelines/tekton/pipeplines_provider.go Outdated
Comment thread k8s/role_bidings.go Outdated
Comment thread k8s/persistent_volumes.go
@zroubalik
Copy link
Copy Markdown
Contributor Author

zroubalik commented Jan 10, 2022

@zroubalik please fix the e2e tests (Error: specified build type "false" is not valid, allowed build types are "disabled", "local" or "git") and generated files ./hack/update-deps.sh.

@matejvasek thanks for the review, I have fixed the problems.

Though ./hack/update-deps.sh or ``./hack/update-codegen.sh` haven't done anything 🤷‍♂️ Not sure how to fix the validation problem.

@matejvasek
Copy link
Copy Markdown
Contributor

Though ./hack/update-deps.sh or ``./hack/update-codegen.sh` haven't done anything 🤷‍♂️ Not sure how to fix the validation problem.

I think that @lkingland had similar issue on macOS.
For me the update removed this file: third_party/VENDOR-LICENSE/github.com/opencontainers/selinux/go-selinux/LICENSE.

@matejvasek
Copy link
Copy Markdown
Contributor

@zroubalik also would it be possible to add some e2e test for in cluster build?

Comment thread k8s/persistent_volumes.go Outdated
@zroubalik
Copy link
Copy Markdown
Contributor Author

@zroubalik also would it be possible to add some e2e test for in cluster build?

Yeah, definitely. But I don't want to bloat this PR. I will create a few follow up issues(tasks) that need to be done for this feature.

Signed-off-by: Matej Vasek <mvasek@redhat.com>
@zroubalik
Copy link
Copy Markdown
Contributor Author

@zroubalik try cherry-picking this: matejvasek@312b6e3.

@matejvasek seems like it helped, thanks a lot :)

@zroubalik
Copy link
Copy Markdown
Contributor Author

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold 🤖 PR should not merge because someone has issued a /hold command. label Jan 10, 2022
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Copy link
Copy Markdown
Member

@lkingland lkingland left a comment

Choose a reason for hiding this comment

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

Looks really good, and I know this represents just the tip of the iceberg!

Comment thread client.go Outdated
Comment thread pipelines/tekton/pipeplines_provider.go Outdated
@knative-prow-robot knative-prow-robot added the lgtm 🤖 PR is ready to be merged. label Jan 11, 2022
@knative-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, zroubalik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [lkingland,zroubalik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@knative-prow-robot knative-prow-robot removed the lgtm 🤖 PR is ready to be merged. label Jan 11, 2022
@zroubalik
Copy link
Copy Markdown
Contributor Author

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold 🤖 PR should not merge because someone has issued a /hold command. label Jan 11, 2022
@zroubalik
Copy link
Copy Markdown
Contributor Author

/hold

need to get #740 merged first

@knative-prow-robot knative-prow-robot added the do-not-merge/hold 🤖 PR should not merge because someone has issued a /hold command. label Jan 11, 2022
Comment thread pipelines/tekton/tasks.go Outdated
@matejvasek
Copy link
Copy Markdown
Contributor

matejvasek commented Jan 12, 2022

When running build user is asked about registry URL, but isn't that known from func.yaml? @zroubalik

Wouldn't it be possible to use existing credential provider? It could read credentials automatically form developer machine, but I am not sure whether it would be good thing.

Comment thread pipelines/tekton/tasks.go Outdated
@zroubalik
Copy link
Copy Markdown
Contributor Author

zroubalik commented Jan 12, 2022

When running build user is asked about registry URL, but isn't that known from func.yaml? @zroubalik

Wouldn't it be possible to use existing credential provider? It could read credentials automatically form developer machine, but I am not sure whether it would be good thing.

@matejvasek wrt registry url, yeah we should try to guess it from registry. And getting credentials automatically would be great, I am not sure if that's possible.

I have captured both of these features in the Task list in #620.
(On-cluster build: Try to get registry credentials from a local developer machine, On-cluster build: Try to suggest correct Server name for Image Registry Credentials prompt)

@matejvasek
Copy link
Copy Markdown
Contributor

And getting credentials automatically would be great, I am not sure if that's possible.

Couldn't we use https://github.com/knative-sandbox/kn-plugin-func/blob/main/docker/creds/credentials.go#L153 @zroubalik ?

@zroubalik
Copy link
Copy Markdown
Contributor Author

And getting credentials automatically would be great, I am not sure if that's possible.

Couldn't we use https://github.com/knative-sandbox/kn-plugin-func/blob/main/docker/creds/credentials.go#L153 @zroubalik ?

Probably yes :) Are you willing to implement this? If so, feel free to covert that task into an issue. It will be more than welcome :)

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@lance
Copy link
Copy Markdown
Contributor

lance commented Jan 12, 2022

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm 🤖 PR is ready to be merged. label Jan 12, 2022
@lance
Copy link
Copy Markdown
Contributor

lance commented Jan 12, 2022

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold 🤖 PR should not merge because someone has issued a /hold command. label Jan 12, 2022
@zroubalik
Copy link
Copy Markdown
Contributor Author

/unhold

@knative-prow-robot knative-prow-robot merged commit cb719ff into knative:main Jan 12, 2022
@lance lance mentioned this pull request Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved 🤖 PR has been approved by an approver from all required OWNERS files. kind/enhancement Feature additions or improvements to existing lgtm 🤖 PR is ready to be merged. size/XXL 🤖 PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

On-cluster build: Code from public Git repository

5 participants