Skip to content

CI add codecov token to the config#312

Merged
BenjaminBossan merged 2 commits intoskops-dev:mainfrom
adrinjalali:codecov
Mar 7, 2023
Merged

CI add codecov token to the config#312
BenjaminBossan merged 2 commits intoskops-dev:mainfrom
adrinjalali:codecov

Conversation

@adrinjalali
Copy link
Copy Markdown
Member

The codecov token we have is not actually used in PRs since it's a token on GH, and they're not exposed to pull_request triggered actions.

The recommended action seems to be adding it hard coded, which doesn't really pose any security risks: codecov/feedback#126

Copy link
Copy Markdown
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Ah I see, it's the same issue again about exposing secrets to forks. I wonder if GH could fix that without leaking the secret, but even if possible, we can't wait for it.

Comment thread .codecov.yml
notify:
after_n_builds: 12
wait_for_ci: true
token: 2b8d4d69-6de6-4e1d-840a-5ccf9d849565
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How about a comment about why it's okay to include the token here?

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.

Added comment.

@adrinjalali
Copy link
Copy Markdown
Member Author

adrinjalali commented Mar 7, 2023

I wonder if GH could fix that without leaking the secret, but even if possible, we can't wait for it.

I don't think it's something GH wants to "fix".

@BenjaminBossan BenjaminBossan merged commit 1e1e844 into skops-dev:main Mar 7, 2023
@BenjaminBossan
Copy link
Copy Markdown
Collaborator

I don't think it's something GH wants to "fix".

Yeah, at this point probably not.

@adrinjalali adrinjalali deleted the codecov branch March 7, 2023 15:14
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