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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Jan 12, 2022
Merged

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

merged 7 commits into from
Jan 12, 2022

Conversation

zroubalik
Copy link
Contributor

@zroubalik zroubalik commented Jan 10, 2022 •
edited by lance

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 10, 2022
@zroubalik zroubalik mentioned this pull request Jan 10, 2022
Merged
@codecov
Copy link

codecov bot commented Jan 10, 2022 •
edited

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 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
feat: support on cluster build from git repo with Tekton
5719574 
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@matejvasek
Copy link
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.

matejvasek
matejvasek reviewed Jan 10, 2022
View reviewed changes
cmd/deploy.go Outdated Show resolved Hide resolved
matejvasek
matejvasek reviewed Jan 10, 2022
View reviewed changes
k8s/role_bidings.go Outdated Show resolved Hide resolved
@zroubalik
Copy link
Contributor Author

zroubalik commented Jan 10, 2022 •
edited

@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
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
Contributor

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

@zroubalik
Copy link
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.

@matejvasek
fixup-licence
163cf64 
Signed-off-by: Matej Vasek <mvasek@redhat.com>
@zroubalik
Copy link
Contributor Author

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

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

@zroubalik
Copy link
Contributor Author

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2022
fix deploy with --build=disabled
2a6d585 
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
lkingland
lkingland approved these changes Jan 11, 2022
View reviewed changes
Copy link
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!

client.go Outdated Show resolved Hide resolved
pipelines/tekton/pipeplines_provider.go Outdated Show resolved Hide resolved
@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2022
@knative-prow-robot
Copy link

[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

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

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

/unhold

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

/hold

need to get #740 merged first

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2022
@matejvasek
Copy link
Contributor

matejvasek commented Jan 12, 2022 •
edited

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.

@zroubalik
Copy link
Contributor Author

zroubalik commented Jan 12, 2022 •
edited

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
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
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 :)

incorporate feedback
c661d6f 
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@lance
Copy link
Member

lance commented Jan 12, 2022

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2022
@lance
Copy link
Member

lance commented Jan 12, 2022

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2022
@zroubalik
Copy link
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
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lance lance lance left review comments

@matejvasek matejvasek matejvasek left review comments

@lkingland lkingland lkingland approved these changes

@salaboy salaboy Awaiting requested review from salaboy

Assignees

@lance lance

@lkingland lkingland

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/enhancement lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

On-cluster build: Code from public Git repository
5 participants
@zroubalik @matejvasek @knative-prow-robot @lance @lkingland