Repository navigation
Conversation
knative-prow
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for knative ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
knative-prow
Bot
added
approved
a5c63d4 to
447a551
Compare
dprotaso
changed the title
knative-prow
Bot
removed
the
do-not-merge/work-in-progress
dsimansk
left a comment
dsimansk
left a comment
There was a problem hiding this comment.
- E2E workflow now tests nothing — test/e2e-tests.sh:34
The script still installs Istio + latest Knative Serving and runs go_test_e2e ./test/e2e, but this PR deleted every test in that package (only an empty package e2e stub remains in test/e2e/main.go). Every presubmit spends minutes standing up a cluster and executes zero tests — green but verifying nothing.
- Rust CloudEvents sample swallows bad PORT — code-samples/serving/cloudevents/cloudevents-rust/src/main.rs:87
port() does value.and_then(|v| v.parse().ok()).unwrap_or(8080), so PORT="8080x" silently binds 8080. Knative's queue-proxy and probes target the injected port, so the revision never goes Ready with no log explaining why. The sibling Vert.x sample rejects bad input — this one should too (or at least log).
- Lost README-vs-source consistency guard — test/sampleconsistency/ (deleted)
The deleted sample-consistency suite enforced that README instructions matched the actual source/Dockerfile/service.yaml. The new samples-test.yaml workflow only runs samples that have a ci.sh and logs "NOT TESTED (no ci.sh)" for the rest (e.g. helloworld-java-spark, helloworld-scala). README drift in those samples now goes uncaught until a user follows broken instructions.
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
This is pretty hard to review in any detail, FWIW. It seems like most of the index.md files have been rewritten as well as moved to README.md, and most of the apps have been rewritten (updated)?
More fundamentally, this makes me wonder whether having sample HTTP server containers for every programming language even makes sense.
| collisions=$(find code-samples -type f -name ci.sh -print0 | | ||
| jq -Rs 'split("\u0000")[:-1] | map(split("/")[-2]) | group_by(.) | map(select(length > 1) | .[0])') | ||
| echo "directories=$(jq -cn --argjson collisions "$collisions" --args ' | ||
| $ARGS.positional | unique | map(split("/") as $path | | ||
| {directory: ., name: (if $collisions | index($path[-1]) | ||
| then $path[1] + "/" + $path[-1] else $path[-1] end)})' "${directories[@]}")" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
I don't understand what "collisions" is doing, and this is some unholy mix of bash and JQ that feels like it should be a real script.
If I'm reading correctly, it looks like "collisions" is supposed to be all the parent directories of ci.sh files where at least two children were changed. It's not not clear to me why we need that, though. (And IMO, cut and uniq would be clearer than map | group_by | map(select).
There was a problem hiding this comment.
This is essentially saying - find all the directories that have ci.sh scripts and compare it to the changed files in the PR. When there is a match then it's a candidate for the samples-e2e test.
There was a problem hiding this comment.
Let me see if we can simplify this
There was a problem hiding this comment.
done - the logic it had before was trying to be smart about handling dirs since we have two code-samples with the same child folder name
eg. serving/helloworld-python and eventing/helloworld-python
I dropped some requirement and it should be simpler now
| matrix: | ||
| include: ${{ fromJSON(needs.discover.outputs.directories) }} | ||
| steps: | ||
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| persist-credentials: false | ||
| - uses: chainguard-dev/actions/setup-kind@8f31e9482b447ff2c69eca944a09b4b31a4a85d0 | ||
| with: | ||
| k8s-version: 1.36.x |
There was a problem hiding this comment.
We're setting up a kind cluster and installing serving once for each changed samples directory? That seems like potentially a lot of clusters.
There was a problem hiding this comment.
Oddly i agree but it seems like github is handling well - eg. see checks in this PR it is running all of the code-sample e2e tests. Those are really just a light smoke test that the docker file builds and it can deploy.
I was expecting dependabot updates to really trigger one or two code-samples at most.
There was a problem hiding this comment.
It feels like we need some meta-documentation (CONTRIBUTING.md or docs/contributing) about how this CI works. If I'm reading it correctly, it spins up a Kind cluster for each sample, loads serving (but not eventing) into the cluster, and then runs a CI script with a port-forwarder to see if whatever is loaded into the cluster is responding / working.
There was a problem hiding this comment.
It should also install eventing for the samples that need it (eg. eventing - python example)
Let me revisit that
Yeah it was either a giant PR and many small ones. The benefit of the big one was coordinating the ci changes and seeing everything pass. Let me know what folks prefer to make reviews easier.
The For example: index.md and README.md.
Yes—by “modernized,” I mean bumping dependencies and updating the code accordingly.
Yeah i never questioned why we have it or who uses it :/ - I'm flexible here. Also note - I'm hoping with the skip-review changes and dependabot + CI checks will make keeping these on auto-pilot. |
447a551 to
703564a
Compare
Summary
Update and simplify code samples, with directory-scoped deployment E2E CI. Commits are organized as CI setup → one commit per sample → shared cleanup.
CI and cleanup
samples-testdiscovers changed sample directories throughci.sh;main()runs build, deploy, and test on kind/Kubernetes 1.36.ci.share explicitly NOT TESTED by CI.Sample changes
Community
TARGET, remove unused test dependencies, and add HTTP smoke CI.Eventing
CloudEvents receivers
Serving
PORT, and support an explicit GitHub API endpoint.PORT, and clarify plaintext/TLS client usage.PORT-aware startup and HTTP smoke CI.PORT, and verify the greeting through HTTP smoke CI.Validation
Retained sample implementations and the sample-CI workflow are unchanged from the fully passing previous run. Checks are rerunning after rebasing onto current
main.Retired samples were removed by the merged #6725, #6726, #6727, #6728, #6729 and #6747; this PR preserves those deletions.