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

feat: func run support all build options #1693

Merged
merged 5 commits into from
May 2, 2023

Conversation

lkingland
Copy link
Member

  • 🎁 func run supports build options and host (non-containerized) build option

Refactors the run command to include all options from build, in the same manner as deploy.
Updates help text to match the other command format, and lays the verbal groundwork for the difference between containerized (currently default) and non-containerized runs.
Adds the --container flag (currently defaults to true) which will, when disabled, invoke the localhost-based runner which, in this commit, is not yet active.

/kind enhancement

@knative-prow knative-prow bot added kind/enhancement do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 25, 2023
@knative-prow
Copy link

knative-prow bot commented Apr 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 25, 2023
@lkingland lkingland force-pushed the scaffolding-run-support branch 2 times, most recently from 0f3754a to 4a3e9a8 Compare April 25, 2023 02:56
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 25, 2023
@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Patch coverage: 80.23% and project coverage change: +0.39 🎉

Comparison is base (d5b4a8d) 62.84% compared to head (c082add) 63.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1693      +/-   ##
==========================================
+ Coverage   62.84%   63.23%   +0.39%     
==========================================
  Files          93       93              
  Lines       12005    12049      +44     
==========================================
+ Hits         7544     7619      +75     
+ Misses       3772     3745      -27     
+ Partials      689      685       -4     
Flag Coverage Δ
e2e-test 39.20% <73.65%> (+0.31%) ⬆️
e2e-test-oncluster 34.72% <46.10%> (+0.08%) ⬆️
e2e-test-oncluster-runtime 29.68% <46.10%> (?)
e2e-test-runtime-go 28.36% <46.10%> (?)
e2e-test-runtime-node 29.51% <46.10%> (?)
e2e-test-runtime-python 29.51% <46.10%> (?)
e2e-test-runtime-quarkus 29.65% <46.10%> (?)
e2e-test-runtime-springboot 28.54% <46.10%> (?)
e2e-test-runtime-typescript 29.65% <46.10%> (?)
integration-tests 50.25% <78.44%> (+0.25%) ⬆️
unit-tests-macos-latest 49.04% <78.44%> (+0.19%) ⬆️
unit-tests-ubuntu-latest 49.97% <78.44%> (+0.24%) ⬆️
unit-tests-windows-latest 49.12% <78.44%> (+0.22%) ⬆️

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

Impacted Files Coverage Δ
cmd/client.go 88.75% <ø> (-0.14%) ⬇️
cmd/root.go 79.47% <ø> (+1.83%) ⬆️
pkg/functions/client.go 65.03% <0.00%> (+0.41%) ⬆️
pkg/functions/function.go 77.92% <ø> (ø)
pkg/functions/job.go 65.21% <0.00%> (-4.56%) ⬇️
cmd/run.go 77.93% <82.69%> (+6.86%) ⬆️
cmd/deploy.go 75.75% <83.33%> (-0.06%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lkingland lkingland force-pushed the scaffolding-run-support branch 2 times, most recently from 0691119 to 656b290 Compare April 25, 2023 05:41
@lkingland lkingland mentioned this pull request Apr 25, 2023
14 tasks
@lkingland lkingland force-pushed the scaffolding-run-support branch 4 times, most recently from cdc2b0f to cad68fc Compare April 25, 2023 14:19
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2023
@lkingland lkingland force-pushed the scaffolding-run-support branch from cad68fc to a1ed87a Compare April 25, 2023 14:27
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2023
@lkingland lkingland force-pushed the scaffolding-run-support branch from a1ed87a to bcb0c0f Compare April 25, 2023 14:52
@lkingland lkingland marked this pull request as ready for review April 26, 2023 00:21
@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 Apr 26, 2023
@knative-prow knative-prow bot requested review from dsimansk and nainaz April 26, 2023 00:21
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2023
Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

Looks great! I just have a couple of nits - and it needs a rebase.

// Another way to think of this is that runs are development-centric tests,
// and thus most likely values changed such as environment variables,
// builder, etc. would not be expected to persist and affect the next deploy.
// Run is ephemeral, deploy is persistent.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should somehow be communicated to the user.

Copy link
Member Author

@lkingland lkingland May 2, 2023

Choose a reason for hiding this comment

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

Yes and at least one time when I was using run locally, specifying --builder, I was confused why it didn't use the same value the next time around.

So on the one hand I am pretty sure that we do not want invocations of run to persist the value of flags which alter deploy (and build), but I also find that once accustomed to the value of flags persisting between commands, I expect them to be consistently persistent.

The only solution that comes to mind is that we actually have a separate set of flag values for running (a development/testing, inner-loop sort of paradigm) vs those used for the deployed function. These would be developer-specific (host scoped) and reside in .func/ in the same way as --remote, etc.

This more complete solution is probably complex enough to warrant its own issue and PR, so for the purposes of this PR I just want to make sure that invoking run with a different builder during development/testing doesn't alter the deployed function configuration!

lkingland and others added 5 commits May 2, 2023 18:38
- Refactors the run command to include all options from build, in the same
  manner as deploy.
- Adds the --container flag (currently defaults to true) which will, when
  disabled, invoke the localhost-based runner which, in this commit, is not
  yet active.
- Updates help text to match the other command format, and lays the verbal
  groundwork for the difference between containerized (currently default)
  and non-containerized runs.
Co-authored-by: Lance Ball <lball@redhat.com>
Co-authored-by: Lance Ball <lball@redhat.com>
Co-authored-by: Lance Ball <lball@redhat.com>
@lkingland lkingland force-pushed the scaffolding-run-support branch from 35bf71f to c082add Compare May 2, 2023 09:38
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2023
Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2023
@knative-prow knative-prow bot merged commit 92d20b0 into knative:main May 2, 2023
lance added a commit to lance/func that referenced this pull request May 2, 2023
The docs were overlooked in the changes to deploy and run in
knative#1693. This commit just updates the
docs so they are consistent with the binary.

Signed-off-by: Lance Ball <lball@redhat.com>
knative-prow bot pushed a commit that referenced this pull request May 2, 2023
The docs were overlooked in the changes to deploy and run in
#1693. This commit just updates the
docs so they are consistent with the binary.

Signed-off-by: Lance Ball <lball@redhat.com>
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants