-
Notifications
You must be signed in to change notification settings - Fork 30
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
True Tokenless for BA and TA #882
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #882 +/- ##
=======================================
Coverage 96.31% 96.32%
=======================================
Files 823 823
Lines 19079 19104 +25
=======================================
+ Hits 18376 18401 +25
Misses 703 703
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a couple of comments on the test results stuff that may also apply to BA
f2f587d
to
609eaf8
Compare
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 2749 tests with View the full list of failed testspytest
|
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
""" | ||
BA sends this in as git_service, TA sends this in as service. | ||
Use this function so this Check class can be used by both views. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably be return validated_data.get("git_service") or validated_data.get("service")
@@ -39,13 +39,13 @@ def has_permission(self, request, view): | |||
class UploadSerializer(serializers.Serializer): | |||
commit = serializers.CharField(required=True) | |||
slug = serializers.CharField(required=True) | |||
service = serializers.CharField(required=False) # git_service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come TA can't use git_service
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's already live in prod I figured it would be a whole thing to switch over to a different var name - what do you think @joseph-sentry ?
upload/views/helpers.py
Outdated
@@ -1,8 +1,13 @@ | |||
import logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file can now be deleted right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can delete get_repository_and_owner_from_slug_and_commit
, my bad I thought I had removed it! ty for catching.
609eaf8
to
e5a7000
Compare
Purpose/Motivation
TT requires certain identifiers, BA and TA don't have the identifiers in their path, so I make custom methods to get the identifiers so that BA and TA can use TT.
Links to relevant tickets
codecov/engineering-team#2298