-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: support new tokenless protocol #447
Conversation
❌ Failed Test Results:Completed 693 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 693 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 693 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 693 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 693 tests with View the full list of failed tests
|
3 similar comments
❌ Failed Test Results:Completed 693 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 693 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 693 tests with View the full list of failed tests
|
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 3455 tests with View the full list of failed tests
|
b03ad4a
to
0de0517
Compare
❌ Failed Test Results:Completed 693 tests with View the full list of failed tests
|
1 similar comment
❌ Failed Test Results:Completed 693 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
1 similar comment
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
if is_fork_pr(pull_dict): | ||
headers = {"X-Tokenless": pull_dict["head"]["slug"], "X-Tokenless-PR": pr} | ||
branch = pull_dict["head"]["slug"] + ":" + branch | ||
tokenless = os.environ.get |
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 think this is missing an arg?
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
1 similar comment
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
1 similar comment
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
568ac88
to
e5f9f17
Compare
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
2 similar comments
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
} | ||
else: | ||
headers = get_token_header_or_fail(token) | ||
headers = get_token_header(token) |
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.
Why are some commands using get_token_header
vs get_token_header_or_fail
? How do we know when to use which?
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.
get_token_header
is for supporting tokenless
, if someone tries to upload using tokenless get_token_header_or_fail
just fails and prevents them from doing anything
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.
Ahh that makes sense, worth leaving another comment here? 😅
if is_fork_pr(pull_dict): | ||
headers = {"X-Tokenless": pull_dict["head"]["slug"], "X-Tokenless-PR": pr} | ||
branch = pull_dict["head"]["slug"] + ":" + branch | ||
tokenless = os.environ.get("TOKENLESS") |
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.
What's this?
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.
gonna leave a comment explaining this but this is how the CLI receives the username of the user to whom the fork belongs to and the branch name from the action
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.
that makes sense, a comment here would be pretty helpful thanks
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.
Some questions, otherwise very excited for this 👀
We no longer have to send the X-Tokenless header anymore. Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
we no longer want to make the github API call to check if the current ref we are uploading from is a PR to determine what to set the tokenless header to. It's no longer necessary and all that is needed for tokenless is to pass the correct branch name (containing a ':') to the create commit step so that tokenless works. The CLI will automatically check if the TOKENLESS env var has been set by the action. The action will set this env var when it detects it's running on a fork. Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
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.
Lgtm! Did you confirm with @thomasrockhu-codecov that this is covered? #447 (comment)
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
4 similar comments
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
4 similar comments
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
4 similar comments
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 691 tests with View the full list of failed tests
|
it's no longer necessary for us to use the
X-Tokenless header to provide tokenless
information to the API, the information in the
regular request bodies will suffice.