Skip to content

fix: restrict python version and update cattrs version restriction #949

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

Merged
merged 6 commits into from
Jan 22, 2022

Conversation

alekseiloginov
Copy link
Contributor

@alekseiloginov alekseiloginov commented Jan 21, 2022

Description

We need to unpin cattrs version to avoid dependency conflicts with other tools using a later version of it.

cattrs version was pinned to 1.1.2 due to https://github.com/Tinche/cattrs/issues/119. Since the issue should be fixed in cattrs 1.3, we update the restriction to >=1.3.

Also python version need to be restricted to <=3.9.9 due to #944. Since Pipfile doesn't support qualifiers for python_full_version, it's set to 3.9.9.

@github-actions
Copy link
Contributor

Typescript Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 7871264. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

APIX Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 7871264. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Codegen Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 7871264. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Python Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 7871264. ± Comparison against base commit ccde3cc.

@joeldodge79
Copy link
Contributor

also, lets change the title to something like the following (to fit conventional commit msg standard)

fix: restrict python version and remove cattrs version

@alekseiloginov alekseiloginov changed the title unpin cattrs version, restrict python to <=3.9.9 fix: restrict python version and remove cattrs version Jan 21, 2022
@alekseiloginov alekseiloginov changed the title fix: restrict python version and remove cattrs version fix: restrict python version and update cattrs version restriction Jan 21, 2022
@github-actions
Copy link
Contributor

Typescript Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit b170a64. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Codegen Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit b170a64. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

APIX Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit b170a64. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Python Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit b170a64. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Typescript Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 7fce8d8. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

APIX Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 7fce8d8. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Codegen Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 7fce8d8. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Python Tests

    4 files  +    4      4 suites  +4   1s ⏱️ -1s
117 tests +117  117 ✔️ +117  0 💤 ±0  0 ❌ ±0 
464 runs  +464  464 ✔️ +464  0 💤 ±0  0 ❌ ±0 

Results for commit 7fce8d8. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

APIX Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit aeb4f61. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Codegen Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit aeb4f61. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Typescript Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit aeb4f61. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Python Tests

    8 files  +    8      8 suites  +8   1m 14s ⏱️ + 1m 14s
140 tests +140  139 ✔️ +139  0 💤 ±0  1 ❌ +1 
742 runs  +742  740 ✔️ +740  0 💤 ±0  2 ❌ +2 

For more details on these failures, see this check.

Results for commit aeb4f61. ± Comparison against base commit ccde3cc.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Codegen Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4ff147a. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

APIX Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4ff147a. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Typescript Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4ff147a. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

Python Tests

  10 files  +  10    10 suites  +10   2m 53s ⏱️ + 2m 53s
140 tests +140  140 ✔️ +140  0 💤 ±0  0 ❌ ±0 
788 runs  +788  788 ✔️ +788  0 💤 ±0  0 ❌ ±0 

Results for commit 4ff147a. ± Comparison against base commit ccde3cc.

@github-actions
Copy link
Contributor

APIX Tests

0 files   -     1  0 suites   - 79   0s ⏱️ - 3m 15s
0 tests  - 320  0 ✔️  - 307  0 💤  - 13  0 ❌ ±0 
0 runs   - 336  0 ✔️  - 323  0 💤  - 13  0 ❌ ±0 

Results for commit 3120bfb. ± Comparison against base commit fa44a8a.

This pull request removes 320 tests.
 DocTitle renders a heading with title prop content ‑  DocTitle renders a heading with title prop content
 MethodBadge renders with the provided verb ‑  MethodBadge renders with the provided verb
ApiSpecSelector it fires a SELECT_SPEC action when another spec is selected ‑ ApiSpecSelector it fires a SELECT_SPEC action when another spec is selected
ApiSpecSelector it lists all available specs ‑ ApiSpecSelector it lists all available specs
ApiSpecSelector the default spec is selected by default ‑ ApiSpecSelector the default spec is selected by default
BrowserAdaptor returns correct font overrides ‑ BrowserAdaptor returns correct font overrides
CodeCopy displays code and clipboard UI ‑ CodeCopy displays code and clipboard UI
CodeDisplay it highlights text matching search pattern ‑ CodeDisplay it highlights text matching search pattern
CodeDisplay it syntax highlights ‑ CodeDisplay it syntax highlights
CodeDisplay utils it can syntax highlight all supported sdk langs ‑ CodeDisplay utils it can syntax highlight all supported sdk langs
…

@github-actions
Copy link
Contributor

Typescript Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 3120bfb. ± Comparison against base commit fa44a8a.

@github-actions
Copy link
Contributor

Codegen Tests

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 3120bfb. ± Comparison against base commit fa44a8a.

@github-actions
Copy link
Contributor

Python Tests

  10 files  +  10    10 suites  +10   2m 55s ⏱️ + 2m 55s
140 tests +140  140 ✔️ +140  0 💤 ±0  0 ❌ ±0 
788 runs  +788  788 ✔️ +788  0 💤 ±0  0 ❌ ±0 

Results for commit 3120bfb. ± Comparison against base commit fa44a8a.

Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

LGTM

@joeldodge79 joeldodge79 self-requested a review January 22, 2022 02:30
@joeldodge79 joeldodge79 merged commit ba28ac6 into main Jan 22, 2022
@joeldodge79 joeldodge79 deleted the unpin_cattrs_version branch January 22, 2022 02:32
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@potiuk
Copy link

potiuk commented Mar 3, 2022

Just one comment here - pinning python version in a library is a very bad idea. What should people do if they already have python 3.9.10 installed ? This - for example - prevents apache/airflow#20882 from merging.

@potiuk
Copy link

potiuk commented Mar 3, 2022

The consequence of this change is that any other package that would be released with looker SDK released with this limitation will not be installable not only on Python 3.9.10 but also any other version it follows. This is extremely bad practice to upper-bound a Python version IMHO. I have never, ever seen a library doing it. It might be fine for an application if it has specific requirements and they are broken (because that application might simply be ok to refuse working on specific python version) but restricting an SDK/library to not being installable on a python version means that you are preventing any other packages that use looker SDK in this version from even installing on newer Python versions.

@joeldodge79
Copy link
Contributor

@potiuk you're correct, it's a less than ideal situation. We're working to resolve the issue but the choice is between pinning it ourselves or letting users find out that it's broken on 3.9.10+

The PR title should have reflected that this is a temporary measure to choose between the lesser of two evils while we find the resources to fix the actual issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants