Explicitly install dependencies in lint env for tox #1456
Conversation
lzchen
requested review from
a team,
owais and
ocelotl
and removed request for
a team
| @@ -10,7 +10,7 @@ env: | |||
| # Otherwise, set variable to the commit of your branch on | |||
| # opentelemetry-python-contrib which is compatible with these Core repo | |||
| # changes. | |||
| CONTRIB_REPO_SHA: b37945bdeaf49822b240281d493d053995cc2b7b | |||
| CONTRIB_REPO_SHA: master | |||
There was a problem hiding this comment.
I still find it weird that core repo has to change this all the time. I think core should always just run contrib master builds just to ensure we don't accidentally break stuff that depends on core. If a contrib PR depends on a core PR, I don't think the core PR needs to run tests from the contrib PR. It should only run master tests to detect regressions. Once the core PR is merged, the author should run tests for the dependent contrib PR again to get it to pass.
- Create core PR, make sure tests for contrib master pass with the change.
- Merge core PR into master.
- Re-run tests for contrib PR so now they pass as the change they depend on in in master now.
In other words, contrib PR may point to a different core branch other than master to ensure tests pass before core PR is merged into master but core PR should never have to account for any code in contrib other than the master branch.
//cc @NathanielRN
There was a problem hiding this comment.
I think core should always just run contrib master builds
If we do this, then changes in Core (i.e. changes to the API) will never be able to pass the tests on Contrib master. This CONTRIB_SHA gives the author a way to prove that they've come up with a solution for Contrib with concrete proof for the sanity of the maintainers merging the PR
just to ensure we don't accidentally break stuff that depends on core.
This CONTRIB_SHA ensures we don't break things in Core. But as a general rule, I understand that Core should be allowed to go forward even if Contrib is breaking.
Once the core PR is merged, the author should run tests for the dependent contrib PR again to get it to pass.
I think this would rely on a lot of trusting they get the Core PR merged correctly the first time 😛 This CONTRIB_SHA just helps prove it is correct through CI.
- Create core PR, make sure tests for contrib master pass with the change.
They won't pass unless the Contrib PR was merged in with failing tests (because the Core PR was not merged yet) and I don't think we ever want them to be in a breaking state?
In other words, contrib PR may point to a different core branch other than master to ensure tests pass before core PR is merged into master but core PR should never have to account for any code in contrib other than the master branch.
I agree with this :) If you read the instructions I wrote in CONTRIBUTING.md section, I wrote this:
- Restore the Core repo PR CONTRIB_REPO_SHA to point to master
The plan was to never change this with the intention of committing it. Naturally it got merged becomes it's not an easy thing to automate and can be tedious to change. I agree that it should always point to master. But I also don't think it's a big deal if this line keeps changing.
In summary: Changing CONTRIB_SHA is just to prove tests pass. And there's no easy way to automatically change it back to master on merge.
| python -m pip install -e {toxinidir}/exporter/opentelemetry-exporter-opencensus[test] | ||
| python -m pip install -e {toxinidir}/exporter/opentelemetry-exporter-otlp[test] | ||
| python -m pip install -e {toxinidir}/exporter/opentelemetry-exporter-prometheus[test] | ||
| python -m pip install -e {toxinidir}/exporter/opentelemetry-exporter-zipkin[test] |
There was a problem hiding this comment.
Sorry I've been heads down on the performance tests. Thanks for adding this change, although I find it weird that we need this (even if pip did upgrade) because isn't this what eachdist.ini sortfirst is supposed to be for?
opentelemetry-python/eachdist.ini
Lines 7 to 15 in 8d195e6
There was a problem hiding this comment.
The issue is more related to the fact that eachdist puts them all into one pip install statement, which apparently is buggy from the recent release.
Builds are failing due to
python scripts/eachdist.py install --editable --with-test-depscommand in the lint env. Apparently appending all packages into one install statement is not working (eachdist.pyconstructs a large install statement likepython -m pip install -e '{toxinidir}/opentelemetry-api[test]' -e '{toxinidir}/opentelemetry-sdk[test]' ....Splitting them up works for some reason. Also,
eachdist.pyis quite complicated, now that our packages are simpler (instrumentations are in the contrib), maybe we can refactor it to make it simpler.