Skip to content

feat: get tool versions without VersionInfo model#7418

Merged
rjsparks merged 3 commits intoietf-tools:mainfrom
jennifer-richards:remove-versioninfo
May 14, 2024
Merged

feat: get tool versions without VersionInfo model#7418
rjsparks merged 3 commits intoietf-tools:mainfrom
jennifer-richards:remove-versioninfo

Conversation

@jennifer-richards
Copy link
Copy Markdown
Member

  • chore: remove update_external_command_info call

  • feat: get tool version without VersionInfo

  • chore: Remove VersionInfo model

  • chore: Migration to remove VersionInfo

  • fix: handle errors better; ignore stderr

* chore: remove update_external_command_info call

* feat: get tool version without VersionInfo

* chore: Remove VersionInfo model

* chore: Migration to remove VersionInfo

* fix: handle errors better; ignore stderr
@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 88.84%. Comparing base (187c2c5) to head (8fcb907).
Report is 119 commits behind head on main.

Files Patch % Lines
ietf/utils/__init__.py 35.71% 9 Missing ⚠️
ietf/submit/checkers.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7418      +/-   ##
==========================================
- Coverage   88.98%   88.84%   -0.15%     
==========================================
  Files         291      292       +1     
  Lines       40717    41068     +351     
==========================================
+ Hits        36233    36486     +253     
- Misses       4484     4582      +98     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment thread ietf/utils/__init__.py
return "Unknown"
elif item not in self._versions:
try:
self._versions[item] = subprocess.run(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One thought that occurs to me is that this puts the time to invoke these 4 things (which should be fast) in the warmup path of every worker thread.

I'm willing to give this a shot, but if we're not already planning to do so in feat/k8s, maybe this should be replaced with something that happens at build time?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, moving to a build time mechanism would be better. I expect that since we generally (or maybe always) actually do work with the tool we ask for the version of, any overhead will be really small by comparison. I guess that's not guaranteed though.

This has the virtue of working both on ietfa and in a container, so it'll bridge the move to the new infrastructure.

@rjsparks rjsparks merged commit a4e0354 into ietf-tools:main May 14, 2024
@jennifer-richards jennifer-richards deleted the remove-versioninfo branch May 15, 2024 13:53
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants