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

allow digested images to be 'run' #2445

Merged
merged 16 commits into from
Aug 28, 2024
Merged

Conversation

gauron99
Copy link
Contributor

@gauron99 gauron99 commented Jul 26, 2024

In order to match the deploy command, make it possible to run digested images as well when building into the container

changes

run (func run) images directly ~ for convenience

Ive added a way to use digested/ valid undigested images to be run using func run cmd. With--build=false one can also run the undigested images directly via skipping the build and using the --image for the image name that they want to deploy.

tests

Ive also added tests that cover which image gets passed on to .Build.Image (which is the image used for the runner) and therefore what image actually gets "run".

new client.Push return value

Added a return value to client.Push which now matches build function (bool indicating if the current push/build was successful or not)

todo:

  • digested image deploys correct image (.Build.Image) when --container=true
  • undigested images direct deploy only with --build=false flag

Copy link

knative-prow bot commented Jul 26, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 26, 2024
@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 26, 2024
Copy link

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 68.96552% with 18 lines in your changes missing coverage. Please review.

Project coverage is 66.73%. Comparing base (4775ad9) to head (4c3bbb4).
Report is 3 commits behind head on main.

Files Patch % Lines
cmd/deploy.go 71.87% 5 Missing and 4 partials ⚠️
cmd/run.go 78.94% 2 Missing and 2 partials ⚠️
pkg/functions/client.go 33.33% 2 Missing and 2 partials ⚠️
cmd/build.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2445      +/-   ##
==========================================
+ Coverage   61.22%   66.73%   +5.51%     
==========================================
  Files         130      130              
  Lines       15354    11950    -3404     
==========================================
- Hits         9400     7975    -1425     
+ Misses       5029     3012    -2017     
- Partials      925      963      +38     
Flag Coverage Δ
e2e-test 36.67% <24.13%> (-0.11%) ⬇️
e2e-test-oncluster 33.28% <18.96%> (-0.03%) ⬇️
e2e-test-oncluster-runtime 29.38% <5.17%> (?)
e2e-test-runtime-go 26.67% <18.96%> (?)
e2e-test-runtime-node 26.06% <18.96%> (?)
e2e-test-runtime-python 26.06% <18.96%> (?)
e2e-test-runtime-quarkus 26.16% <18.96%> (?)
e2e-test-runtime-rust 25.17% <18.96%> (?)
e2e-test-runtime-springboot 25.24% <18.96%> (?)
e2e-test-runtime-typescript 26.15% <18.96%> (?)
integration-tests 52.78% <68.96%> (+2.37%) ⬆️
unit-tests 50.97% <68.96%> (?)
unit-tests-macos-latest ?
unit-tests-ubuntu-latest ?
unit-tests-windows-latest ?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gauron99 gauron99 marked this pull request as ready for review August 7, 2024 08:06
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2024
@gauron99 gauron99 requested review from lkingland and matejvasek and removed request for jrangelramos and vyasgun August 7, 2024 08:06
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.

I particularly like the new unit tests.

Looks great as-is! I added a couple of questions/suggestions.

/hold for optional changes from review comments

cmd/deploy.go Outdated Show resolved Hide resolved
cmd/deploy_test.go Outdated Show resolved Hide resolved
cmd/deploy_test.go Show resolved Hide resolved
cmd/run.go Outdated Show resolved Hide resolved
cmd/run.go Outdated Show resolved Hide resolved
ctx, cancel := context.WithTimeout(context.Background(), time.Minute*10)
t.Cleanup(cancel)

image := testImageWithDigest
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to first build the image, get its digested image name, and then use that digested image for testing "func run" (from a new temporary directory such that the build stamp from .func is not considered)? That would slow the test a bit, but would remove the need for a pre-built external container for the test

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2024
@knative-prow knative-prow bot added lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 13, 2024
Copy link

linux-foundation-easycla bot commented Aug 26, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 26, 2024
@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 26, 2024
Signed-off-by: David Fridrich <[email protected]>
Signed-off-by: David Fridrich <[email protected]>
@lkingland
Copy link
Member

/approve

Looking good

Signed-off-by: David Fridrich <[email protected]>
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2024
Copy link

knative-prow bot commented Aug 27, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gauron99
Copy link
Contributor Author

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 28, 2024
@knative-prow knative-prow bot merged commit ca61712 into knative:main Aug 28, 2024
39 checks passed
matejvasek pushed a commit to matejvasek/faas that referenced this pull request Sep 4, 2024
* init fix

Signed-off-by: gauron99 <[email protected]>

* dont override direct deploy tag, more tests

Signed-off-by: gauron99 <[email protected]>

* fix

Signed-off-by: gauron99 <[email protected]>

* dont validate with tagged image, fix comment

Signed-off-by: gauron99 <[email protected]>

* init run fix for --image

Signed-off-by: gauron99 <[email protected]>

* init

Signed-off-by: gauron99 <[email protected]>

* int test, add valid untdigested images to run

Signed-off-by: gauron99 <[email protected]>

* check images passed to runner for func run command

Signed-off-by: gauron99 <[email protected]>

* fix build/deploy image passing bug add test

Signed-off-by: gauron99 <[email protected]>

* fix

Signed-off-by: gauron99 <[email protected]>

* remove extra printing

Signed-off-by: gauron99 <[email protected]>

* merge functions to digested

Signed-off-by: gauron99 <[email protected]>

* misspell

Signed-off-by: gauron99 <[email protected]>

* simplify

Signed-off-by: David Fridrich <[email protected]>

* quick fix

Signed-off-by: David Fridrich <[email protected]>

* remove prints, comment

Signed-off-by: David Fridrich <[email protected]>

---------

Signed-off-by: gauron99 <[email protected]>
Signed-off-by: David Fridrich <[email protected]>
knative-prow bot pushed a commit that referenced this pull request Sep 5, 2024
* test: adding GO s2i to e2e test suite (#2401)

* Remove superfluous equals sign from envvar (#2446)

* Remove superfluous equals sign from envvar

Signed-off-by: Matej Vašek <[email protected]>

* Update rust templates Cargo.lock

Signed-off-by: Matej Vašek <[email protected]>

---------

Signed-off-by: Matej Vašek <[email protected]>

* allow digested images to be 'run' (#2445)

* init fix

Signed-off-by: gauron99 <[email protected]>

* dont override direct deploy tag, more tests

Signed-off-by: gauron99 <[email protected]>

* fix

Signed-off-by: gauron99 <[email protected]>

* dont validate with tagged image, fix comment

Signed-off-by: gauron99 <[email protected]>

* init run fix for --image

Signed-off-by: gauron99 <[email protected]>

* init

Signed-off-by: gauron99 <[email protected]>

* int test, add valid untdigested images to run

Signed-off-by: gauron99 <[email protected]>

* check images passed to runner for func run command

Signed-off-by: gauron99 <[email protected]>

* fix build/deploy image passing bug add test

Signed-off-by: gauron99 <[email protected]>

* fix

Signed-off-by: gauron99 <[email protected]>

* remove extra printing

Signed-off-by: gauron99 <[email protected]>

* merge functions to digested

Signed-off-by: gauron99 <[email protected]>

* misspell

Signed-off-by: gauron99 <[email protected]>

* simplify

Signed-off-by: David Fridrich <[email protected]>

* quick fix

Signed-off-by: David Fridrich <[email protected]>

* remove prints, comment

Signed-off-by: David Fridrich <[email protected]>

---------

Signed-off-by: gauron99 <[email protected]>
Signed-off-by: David Fridrich <[email protected]>

* On cluster s2i build for Go (alternative version) (#2471)

* Prepare util image to accomodate multiple cmds

As of now func-util image has only one command of our own -- "deploy".
This commits prepares grounds for one additional command named "scaffolding".
The commands will be implemented in one binary and dispatched over
argv[0] using symlinks. Kinda like busybox does.

Signed-off-by: Matej Vašek <[email protected]>

* Add scaffolding for on-cluster build

Added new build step in tekton pipeline that scaffolds main() for Go
porject when using s2i builder.

Signed-off-by: Matej Vašek <[email protected]>

* Better docker build caching

This will cache dependencies between docker builds.

Signed-off-by: Matej Vašek <[email protected]>

* fixup: perms

Signed-off-by: Matej Vašek <[email protected]>

* fixup: remove broken check

Signed-off-by: Matej Vašek <[email protected]>

* fixup: remove test because of 'no space left on device'

Signed-off-by: Matej Vašek <[email protected]>

* Make func-util rootfull

Signed-off-by: Matej Vašek <[email protected]>

---------

Signed-off-by: Matej Vašek <[email protected]>

* Incorporate s2i CLI into func-utils image. (#2473)

Signed-off-by: Matej Vašek <[email protected]>

* chore: update Quarkus platform version to 3.14.1 (#2474)

Co-authored-by: Knative Automation <[email protected]>
Signed-off-by: Matej Vašek <[email protected]>

* chore: update Springboot platform version to 3.3.3 (#2465)

Co-authored-by: Knative Automation <[email protected]>
Signed-off-by: Matej Vašek <[email protected]>

---------

Signed-off-by: Matej Vašek <[email protected]>
Signed-off-by: gauron99 <[email protected]>
Signed-off-by: David Fridrich <[email protected]>
Co-authored-by: Jefferson Ramos <[email protected]>
Co-authored-by: David Fridrich <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Knative Automation <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants