-
Notifications
You must be signed in to change notification settings - Fork 36
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: adjust for tokenless v3 #434
Conversation
❌ Failed Test Results:Completed 666 tests with View the full list of failed tests
|
3 similar comments
❌ Failed Test Results:Completed 666 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 666 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 666 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 2668 tests with View the full list of failed tests
|
627866b
to
653ef1c
Compare
❌ Failed Test Results:Completed 666 tests with View the full list of failed tests
|
3 similar comments
❌ Failed Test Results:Completed 666 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 666 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 666 tests with View the full list of failed tests
|
codecov_cli/commands/report.py
Outdated
@@ -13,15 +13,6 @@ | |||
@click.option( | |||
"--code", help="The code of the report. If unsure, leave default", default="default" | |||
) | |||
@click.option( |
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 is a breaking change. I think that users that run the CLI in a workflow_dispatch
need 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.
I just don't know if it makes sense for the create-report command
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.
ahhh I see, that does make sense. however, we should probably for now make it a deprecated
field
@@ -95,6 +96,15 @@ def get_token_header_or_fail(token: str) -> dict: | |||
return {"Authorization": f"token {token}"} | |||
|
|||
|
|||
def get_auth_header(token): | |||
tokenless = os.environ.get("TOKENLESS", None) |
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.
are we making this change for the orb and step too? Why doesn't the CLI just do this itself?
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 don't think the CLI has access to the same context as the action but i can double check, but you are correct we should make a similar change the the orb and step as well
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
3 similar comments
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
@@ -18,7 +18,7 @@ | |||
"--pr", | |||
"--pull-request-number", | |||
"pull_request_number", | |||
help="Specify the pull request number mannually. Used to override pre-existing CI environment variables", | |||
help="Deprecated option, this option has no effect on the execution of this command anymore. Specify the pull request number mannually. Used to override pre-existing CI environment variables", | |||
cls=CodecovOption, | |||
fallback_field=FallbackFieldEnum.pull_request_number, |
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.
perhaps we don't need this anymore either
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
3 similar comments
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
we are no longer sending the X-Tokenless-PR header and we are changing the format of the contents of the X-Tokenless header Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
Co-authored-by: Tom Hu <88201630+thomasrockhu-codecov@users.noreply.github.com>
4e8013d
to
4fbd6ec
Compare
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
1 similar comment
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
1 similar comment
❌ Failed Test Results:Completed 667 tests with View the full list of failed tests
|
pull_dict = get_pull(service, decoded_slug, pr) if not token else None | ||
if is_fork_pr(pull_dict): | ||
headers = {"X-Tokenless": pull_dict["head"]["slug"], "X-Tokenless-PR": pr} | ||
branch = pull_dict["head"]["slug"] + ":" + branch |
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.
don't we still need this?
we are no longer sending the X-Tokenless-PR header and we are changing the format of the contents of the X-Tokenless header
The X-Tokenless header will now be set to
<fork user name>:<branch name>
and will be the only extra header we send for tokenless requests. We get the value of this header from the TOKENLESS environment variable that will be set by the Codecov action.