-
Notifications
You must be signed in to change notification settings - Fork 136
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
feat!: support on cluster build from git repo with Tekton #743
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
b83a80e
to
20ef9f3
Compare
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@zroubalik please fix the e2e tests ( |
@matejvasek thanks for the review, I have fixed the problems. Though |
I think that @lkingland had similar issue on macOS. |
@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>
@matejvasek seems like it helped, thanks a lot :) |
/hold |
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
There was a problem hiding this 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!
[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:
Approvers can indicate their approval by writing |
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
/unhold |
/hold need to get #740 merged first |
When running build user is asked about registry URL, but isn't that known from 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. |
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>
/lgtm |
/unhold |
/unhold |
Signed-off-by: Zbynek Roubalik zroubali@redhat.com
Changes
func deploy --build
originally acceptedtrue
orfalse
, now it acceptsdisabled
,local
orgit
values/kind enhancement
on-cluster-git.mp4
Fixes: #748