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

Define git_service config getter for Github actions #376

Closed
wants to merge 1 commit into from

Conversation

kdarkhan
Copy link

@kdarkhan kdarkhan commented Feb 7, 2024

This is my attempt at fixing the issue google/mdbook-i18n-helpers#173.

I am not very familiar with this codebase but I think it might fix the issue.

My Github workflows implements an unusual technique for codecov uploads which I described in this thread.

It used to work in codecov-action v3 and stopped working in v4. The error I am getting is Commit creating failed: ["Service not found: none"].

I suspect this is due to missing implementation of _get_git_service in github_actions.py.

@kdarkhan
Copy link
Author

kdarkhan commented Feb 8, 2024

I just discovered codecov/feedback#263

Based on codecov/feedback#263 (comment), my PR might fix the issue.

Ok, I see now that is not that simple. The service could be either github or github_enterprise.

If my approach is wrong, it would be nice to expose git-service param in the codecov-action.

@kdarkhan
Copy link
Author

@drazisil-codecov could this PR fix the issue that you mentioned in this comment?

@drazisil-codecov
Copy link
Contributor

drazisil-codecov commented Feb 22, 2024

@drazisil-codecov could this PR fix the issue that you mentioned in this comment?

It might, but as you observed , that doesn't help github_enterprise. However, "enterprise" in this context refers to self-hosted, or a URL that isn't github.com. I think that if it could be aware of if a URL was also passed, that should be good.

Also tests, please , if possible.

I'll refer to @thomasrockhu-codecov since I don't really touch the action code that much.

@kdarkhan
Copy link
Author

Thanks for response, @drazisil-codecov.

I will wait for confirmation from @thomasrockhu-codecov on the approach. If this approach is correct I can add tests as well. Did not want to invest too much work into this since I wasn't sure if this is the right fix.

@kdarkhan
Copy link
Author

Based on codecov/feedback#263 (comment), this is not needed. The fix will be done differently.

@kdarkhan kdarkhan closed this Feb 26, 2024
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