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

docs: add a language pack "contract" document #918

Merged
merged 4 commits into from
Apr 11, 2022

Conversation

lance
Copy link
Member

@lance lance commented Mar 22, 2022

Rendered version: https://github.com/lance/func/blob/lance/add-language-pack-contract/docs/language-pack-providers/language-pack-contract.md

The docs/guides/langugage-packs.md document does a good job of describing what's required of a language pack in terms of directory structure and the options for the manifest.yaml file. But it does a pretty crappy job of describing the whole thing in context. I've tried to do that here.

Also some fixes to links and reorganization of the docs to make things a little easier to find.

Fixes: #574 (maybe?)

Signed-off-by: Lance Ball lball@redhat.com

@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 22, 2022
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance

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-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2022
@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #918 (4b4129a) into main (739cded) will increase coverage by 1.98%.
The diff coverage is 29.13%.

@@            Coverage Diff             @@
##             main     #918      +/-   ##
==========================================
+ Coverage   43.85%   45.83%   +1.98%     
==========================================
  Files          55       57       +2     
  Lines        5163     7442    +2279     
==========================================
+ Hits         2264     3411    +1147     
- Misses       2576     3703    +1127     
- Partials      323      328       +5     
Impacted Files Coverage Δ
cmd/completion_util.go 0.00% <0.00%> (ø)
cmd/invoke.go 34.81% <0.00%> (-0.97%) ⬇️
docker/docker_client.go 82.52% <ø> (+0.57%) ⬆️
k8s/logs.go 0.00% <0.00%> (ø)
k8s/secrets.go 13.54% <0.00%> (-0.75%) ⬇️
s2i/builder.go 32.69% <32.69%> (ø)
client.go 61.94% <50.00%> (+2.01%) ⬆️
invoke.go 52.99% <61.53%> (+2.99%) ⬆️
cmd/build.go 60.49% <70.83%> (+0.86%) ⬆️
cmd/root.go 72.28% <100.00%> (+1.66%) ⬆️
... and 55 more

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 48d081b...4b4129a. Read the comment docs.

@lance lance marked this pull request as draft March 22, 2022 20:05
@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 22, 2022
@lance
Copy link
Member Author

lance commented Mar 22, 2022

Converting to draft, as it's still rough and unfinished in a couple of spots - still looking for feedback.
Ready for review

@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 25, 2022
@lance lance force-pushed the lance/add-language-pack-contract branch from 6fb4d47 to 837fb60 Compare April 6, 2022 18:59
@lance lance marked this pull request as ready for review April 6, 2022 19:00
@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 6, 2022
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.

Hugely beneficial addition! All I could come up with was a bunch of minor syntactic nits.

👍🏻

/hold for others

docs/function-developers/developers_guide.md Show resolved Hide resolved
docs/function-developers/developers_guide.md Outdated Show resolved Hide resolved
docs/function-developers/developers_guide.md Show resolved Hide resolved
docs/function-developers/developers_guide.md Outdated Show resolved Hide resolved
docs/function-developers/developers_guide.md Outdated Show resolved Hide resolved
docs/language-pack-providers/language-pack-contract.md Outdated Show resolved Hide resolved
docs/language-pack-providers/language-pack-contract.md Outdated Show resolved Hide resolved
docs/language-pack-providers/language-pack-contract.md Outdated Show resolved Hide resolved
docs/language-pack-providers/language-pack-contract.md Outdated Show resolved Hide resolved
docs/language-pack-providers/language-pack-contract.md Outdated Show resolved Hide resolved
@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 Apr 8, 2022
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2022
@knative-prow
Copy link

knative-prow bot commented Apr 8, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance, 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 removed the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2022
The docs/guides/langugage-packs.md document does a good job of describing
what's required of a language pack in terms of directory structure and the
options for the `manifest.yaml` file. But it does a pretty crappy job of
describing the whole thing in context. I've tried to do that here.

Also some fixes to links and reorganization of the docs to make things a
little easier to find.

Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
@lance lance force-pushed the lance/add-language-pack-contract branch from f5ce1d6 to 5991cc5 Compare April 8, 2022 14:24
Signed-off-by: Lance Ball <lball@redhat.com>
@lance
Copy link
Member Author

lance commented Apr 8, 2022

@lkingland thanks for the thorough review. I've incorporated your feedback, with a couple of replies to your comments above. I've also inlined the language pack reference as suggested. It is mostly a copy/paste, but I did tweak some of the wording here and there to try and make things clearer. PTAL

Signed-off-by: Lance Ball <lball@redhat.com>
@lkingland
Copy link
Member

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2022
@lance
Copy link
Member Author

lance commented Apr 11, 2022

/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 Apr 11, 2022
@knative-prow knative-prow bot merged commit 76c647a into knative:main Apr 11, 2022
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. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create some documentation around what a func language template looks like
3 participants