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

Update shared version #43

Merged
merged 3 commits into from
Aug 1, 2023
Merged

Update shared version #43

merged 3 commits into from
Aug 1, 2023

Conversation

scott-codecov
Copy link
Contributor

Updates the shared version and accommodates changes from:

@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #43 (17fbdfb) into main (31a0d91) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
- Coverage   98.63%   98.62%   -0.02%     
==========================================
  Files         355      355              
  Lines       25681    25682       +1     
==========================================
- Hits        25331    25328       -3     
- Misses        350      354       +4     
Flag Coverage Δ
integration 98.59% <100.00%> (-0.02%) ⬇️
latest-uploader-overall 98.59% <100.00%> (-0.02%) ⬇️
onlysomelabels 98.61% <100.00%> (-0.02%) ⬇️
unit 98.59% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 97.35% <100.00%> (-0.04%) ⬇️
OutsideTasks 98.36% <ø> (ø)
Files Changed Coverage Δ
tasks/sync_repos.py 95.42% <100.00%> (-2.61%) ⬇️
tasks/tests/unit/test_sync_repos_task.py 100.00% <100.00%> (ø)
Related Entrypoints
run/app.tasks.sync_repos.SyncRepos

@matt-codecov
Copy link
Contributor

matt-codecov commented Jul 31, 2023

re codecov/shared#13, want to get a second opinion on a question before merging:

i suppose this means that, if i fork upstream rust, my fork will be shown in the UI but upstream rust won't be anymore. is that a big change?

EDIT: actually i think the current UX doesn't show the upstream repo either. the current UX groups by org/user, and upstream Rust isn't part of my own set or those of my orgs. if i log into Codecov and show the matt-codecov section, it has a rust repo but it does not have mhammerly/rust that it was forked from.

Copy link
Contributor

@matt-codecov matt-codecov left a comment

Choose a reason for hiding this comment

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

no blocking concerns, just consider the two Qs before merging. thank you!

@@ -89,7 +89,7 @@ async def sync_repos_using_integration(self, db_session, git, ownerid, username)
with metrics.timer(f"{metrics_scope}.sync_repos_using_integration.list_repos"):
repos = await git.list_repos_using_installation(username)
if repos:
service_ids = {repo["id"] for repo in repos}
service_ids = {str(repo["repo"]["service_id"]) for repo in repos}
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we just make this a string in the return format for both this and list_repos()? i don't think we use them anywhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change in shared and updated things here accordingly

Copy link
Contributor

@matt-codecov matt-codecov left a comment

Choose a reason for hiding this comment

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

also great catch re: .get() vs []

@scott-codecov scott-codecov merged commit 85ab448 into main Aug 1, 2023
4 of 6 checks passed
@scott-codecov scott-codecov deleted the scott/update-shared branch August 1, 2023 13:33
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.

2 participants