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.

Sign up for GitHub

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

Jump to bottom

Explicitly install dependencies in lint env for tox #1456

Merged
merged 2 commits into from
Dec 8, 2020
Merged

Explicitly install dependencies in lint env for tox #1456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72a434b
lint
lzchen Dec 8, 2020
b8ea3aa
test
lzchen Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

  1. Create core PR, make sure tests for contrib master pass with the change.
  2. Merge core PR into master.
  3. 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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

  1. 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:

  1. 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.


jobs:
build:
Expand Down
12 changes: 11 additions & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,17 @@ deps =
httpretty

commands_pre =
python scripts/eachdist.py install --editable --with-test-deps
python -m pip install -e {toxinidir}/opentelemetry-api[test]
python -m pip install -e {toxinidir}/opentelemetry-sdk[test]
python -m pip install -e {toxinidir}/opentelemetry-instrumentation[test]
python -m pip install -e {toxinidir}/opentelemetry-proto[test]
python -m pip install -e {toxinidir}/tests/util[test]
python -m pip install -e {toxinidir}/instrumentation/opentelemetry-instrumentation-opentracing-shim[test]
python -m pip install -e {toxinidir}/exporter/opentelemetry-exporter-jaeger[test]
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]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

sortfirst=
opentelemetry-api
opentelemetry-sdk
opentelemetry-instrumentation
opentelemetry-proto
tests/util
instrumentation/*
exporter/*
ext/*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


commands =
python scripts/eachdist.py lint --check-only
Expand Down