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

Add version check #1711

Merged
merged 5 commits into from
May 8, 2024
Merged

Add version check #1711

merged 5 commits into from
May 8, 2024

Conversation

ternaus
Copy link
Collaborator

@ternaus ternaus commented May 7, 2024

No description provided.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @ternaus - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 4 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
albumentations/check_version.py Outdated Show resolved Hide resolved
albumentations/check_version.py Outdated Show resolved Hide resolved
albumentations/check_version.py Show resolved Hide resolved
@ternaus ternaus merged commit 9386ad0 into main May 8, 2024
17 checks passed
@ternaus ternaus deleted the add_version_check branch May 8, 2024 00:57
@Antinomy20001
Copy link

Antinomy20001 commented May 14, 2024

We have noticed that the check_for_updates function only handles exceptions for certain cases (network exceptions). It does not fully cover the scenarios in complex network environments. However, this function will be called whenever 'import albumentations' is executed, which indicates its incompleteness.

def check_for_updates() -> None:

eg:

  • data = response.read() but response read timeout
  • json_data = json.loads(data.decode(encoding)) but decoded data is not a json
  • latest_version = json_data["info"]["version"] but there are Key Errors in json_data

Considering complex scenarios such as intranet environments, hijacking, and reverse proxies, it is not sufficient to merely handle exceptions related to the network connection itself.

@ternaus
Copy link
Collaborator Author

ternaus commented May 14, 2024

Thanks.

Will make exception general

Correct me, if I am wrong, but regardless of the exception type, nothing happens as it is just a pass after that.

@ternaus
Copy link
Collaborator Author

ternaus commented May 14, 2024

Made exception more general in #1725

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.

None yet

2 participants