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.

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

PyPI version check #475

Merged
merged 23 commits into from
Feb 8, 2019
Merged

PyPI version check #475

merged 23 commits into from
Feb 8, 2019

Conversation

asvetlov
Copy link
Contributor

@asvetlov asvetlov commented Feb 7, 2019

The code is dirty somewhere but better structure requires config refactoring.
Let's do it in a separate PR.

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@a660a47). Click here to learn what that means.
The diff coverage is 97.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #475   +/-   ##
=========================================
  Coverage          ?   90.61%           
=========================================
  Files             ?       36           
  Lines             ?     2439           
  Branches          ?      282           
=========================================
  Hits              ?     2210           
  Misses            ?      178           
  Partials          ?       51
Impacted Files Coverage Δ
python/neuromation/cli/rc.py 93.9% <100%> (ø)
python/neuromation/cli/version_utils.py 100% <100%> (ø)
python/neuromation/cli/utils.py 96.99% <100%> (ø)
python/neuromation/cli/main.py 55.02% <50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a660a47...dbf9fa7. Read the comment docs.

@shagren
Copy link
Contributor

shagren commented Feb 7, 2019

Guys, why not send CLI version to api(via user-agent header) and check via server side?
Maybe it's another case.

@asvetlov
Copy link
Contributor Author

asvetlov commented Feb 7, 2019

Do you propose to move PyPI request from neuro client to one of (which?) our servers?
Why?
It complicates the logic without any visible benefit.

@shagren
Copy link
Contributor

shagren commented Feb 7, 2019

Do you propose to move PyPI request from neuro client to one of (which?) our servers?

Just add to Http client right user-agent header, like 'neuro/0.2.dev'

Why?

API can throw exception if client too old for parsing response. I know that better to use versioning in url(/v1/) but my solution is more cheap at this moment.

It complicates the logic without any visible benefit.

  • it's cheap
  • we can force users to upgrade client in any moment
  • we can gather some statistic in future

I think my idea is not opposite to this PR, it side feature

@asvetlov
Copy link
Contributor Author

asvetlov commented Feb 7, 2019

From my understanding, you are proposing to send client version in User-Agent for every request to our servers.

What servers should do? Should we modify all our existing entrypoints (platform, registry, auth, and storage) to process the header?
Should every entrypoint have a list of compatible client versions?
How to make a hint that the migration is not required but very desirable?

@shagren
Copy link
Contributor

shagren commented Feb 8, 2019

From my understanding, you are proposing to send client version in User-Agent for every request to our servers.

Yes

What servers should do? Should we modify all our existing entrypoints (platform, registry, auth, and storage) to process the header?

Yes, we need to modify our existing entrypoints. Servers must check client version.

Should every entrypoint have a list of compatible client versions?

Yes. It`s can be one global value or different for each per end-point.

How to make a hint that the migration is not required but very desirable?

We can have two variables: required_version, recommended_version

It's can be decorator like: @required(0.2), @recommended(0.3.1)

@asvetlov
Copy link
Contributor Author

asvetlov commented Feb 8, 2019

Sorry, modifying all endpoints for all servers is not an option. Especially manual modifying.

@asvetlov asvetlov merged commit e3705c2 into master Feb 8, 2019
@asvetlov asvetlov deleted the ay/version-check branch February 8, 2019 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants