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

fix Codefresh PR number variable #114

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

snackattas
Copy link
Contributor

@snackattas snackattas commented Mar 18, 2024

⚡ Summary

This PR switches from the Pull Request ID, which is an internal Github ID, to the Pull Request Number, for Codefresh, which will make these links work:
2024-03-18 09 28 17

https://codefresh.io/docs/docs/pipelines/variables/

Here's a similar issue with gitlab that I found, where they also switched over to the github PR number from PR id

☑️ Checklist

  • Add specs

@mike-burns
Copy link
Contributor

We pulled that in from node-coveralls, which still has this discrepency.


In the Coveralls app code, it is used from that link you showed, and it's also used for API calls to GitHub, GitLab, BitBucket, and Stash. I can confirm that those APIs -- the ones we use anyway -- expect a "PR number"-like value, and not an ID.

Looks good to me.

Copy link

coveralls-official bot commented Apr 8, 2024

Pull Request Test Coverage Report for Build 8608271581

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 93.848%

Files with Coverage Reduction New Missed Lines %
src/coverage_reporter/api/jobs.cr 1 97.92%
Totals Coverage Status
Change from base Build 8608261724: -0.1%
Covered Lines: 900
Relevant Lines: 959

💛 - Coveralls

@snackattas
Copy link
Contributor Author

@mike-burns

In the Coveralls app code, it is used from that link you showed, and it's also used for API calls to GitHub, GitLab, BitBucket, and Stash. I can confirm that those APIs -- the ones we use anyway -- expect a "PR number"-like value, and not an ID.

They are both numbers though...both integers, so I don't know how one could distinguish one from the other

It's just, one's an internal Github ID, and one corresponds to the Github PR number, so you can build URL links based on it

@mike-burns mike-burns merged commit b35c4ea into coverallsapp:master Apr 9, 2024
10 of 11 checks passed
@mike-burns
Copy link
Contributor

Sorry, that paragraph was an explanation clarifying that I checked the Coveralls backend and confirmed that this PR is correct.

@afinetooth
Copy link
Member

@snackattas just doing some housekeeping to confirm this had been released and is working for you.

Thanks, @mike-burns!

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.

4 participants