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

Bug fixes to #718 #725

Merged
merged 5 commits into from
Mar 24, 2024
Merged

Bug fixes to #718 #725

merged 5 commits into from
Mar 24, 2024

Conversation

jhutchings1
Copy link
Contributor

Fixes #718
This pull request addresses the handling of GitHub Actions in the scorecard.ts and scorecard.test.ts files. The changes include the addition of a special case for GitHub Actions in the getScorecardLevels function, modification of the apiRoot URL, and the addition of new tests to verify these changes.

@jhutchings1 jhutchings1 requested a review from a team as a code owner March 22, 2024 21:51
const parts = packageName.split('/')
repositoryUrl = `github.com/${parts[0]}/${parts[1]}` // e.g. github.com/actions/checkout
}

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 nit: would be good to explicitly check this output in the new test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, the repositoryUrl isn't exposed outside of the function. Thoughts on how to bootstrap that into the tests? Was trying to avoid side effects on the Change object by using a local variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry late getting back to this. May the easiest play is to factor out 21-26 into a function that accepts a Change and outputs a repositoryUrl (or not) that line 29 can check, then test that in isolation with some Actions-flavored Changes?

Again not the end of the world, since it's already landed 👍

Copy link
Contributor

@elireisman elireisman left a comment

Choose a reason for hiding this comment

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

Nice change! One callout but happy to restamp after an update if you agree

@febuiles febuiles merged commit 9093495 into main Mar 24, 2024
6 checks passed
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.

Invalid URL for OpenSSF Scorecard Package
3 participants