Modify workflows to test on java17 #911
Conversation
There was a problem hiding this comment.
General guidance here from a review point of view. I find smaller mistakes harder to find with PRs that are mostly refactoring like this one. I would advise to keep PRs like this tightly scoped around one thing. You chose to include changes to the python workflows also which polluted the PR imo.
| sample-app: [ aws-sdk, okhttp ] | ||
| instrumentation-type: [ wrapper ] | ||
| confmap: [ "noop" ] | ||
| include: |
There was a problem hiding this comment.
The include statement here was a bit hard to grok for me. Would it be easier to invert this by adding all possible values in the the matrix key values, and then using exclude instead to remove combinations we don't want? Exclude could™ be easier to read because it only requires partial matches.
It seems like most of our logic here is just excluding okhttp pairing with agent and agent-confmap
There was a problem hiding this comment.
yeah, we would also want to exclude agent-conf map to run on different runtimes of java Since agent and agent-confmap are the same tests and confmap functionality tested in single runtime is good enough. i will update the PR.
There was a problem hiding this comment.
Ahh I looked into it seems like usage of include is better in this case. The agent-confmap will include conf map for http, https and s3. So even if we have to use exclude, unless we add instrumentation-type: agent in the matrix. we have to use include again.
correct me if my understanding is wrong.
yeah my bad, i thought to clean up a little up and also aimed to get alarmed when the workflow fails. |
Description:
Changes were made to do java workflow to test on multiple run times for now with refactoring.
Test run that only ensures the tests run as expected.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.