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

✨ Add line number to unpinned dependency: GitHub workflow "uses" field #821

Merged
merged 5 commits into from
Aug 30, 2021

Conversation

ristomcgehee
Copy link
Contributor

@ristomcgehee ristomcgehee commented Aug 8, 2021

  • Please check if the PR fulfills these requirements
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Feature

  • What is the current behavior? (You can also link to an open issue here)
    Line numbers are not displayed in the results when there are unpinned dependencies.
    Feature: add line number to Pinned-Dependencies detail reporting #725

  • What is the new behavior (if this is a feature change)?
    When there is an unpinned uses field in a .github/workflows file , the line number for the uses field will be displayed in the detailed results.
    I will create additional PRs to add line numbers for other places in the code that deal with unpinned dependencies.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No

  • Other information:
    I updated gopkg.in/yaml to v3 in checks/pinned_dependencies.go in order to use the newer UnmarshalYAML() in order to set the line number of the field. I did not update other files to go from gopkg.in/yaml/v2 to ``gopkg.in/yaml/v3` because when I tried doing so, some of the test for Token-Permissions failed.

Copy link
Member

@naveensrinivasan naveensrinivasan left a comment

Choose a reason for hiding this comment

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

Thanks,Could we add some tests to this?

@ristomcgehee
Copy link
Contributor Author

Thanks,Could we add some tests to this?

Sure thing, I'll get some added.

@ristomcgehee
Copy link
Contributor Author

Tests have been added.

checks/pinned_dependencies.go Outdated Show resolved Hide resolved
@@ -542,3 +543,78 @@ func TestGitHubWorflowRunDownload(t *testing.T) {
})
}
}

func TestGitHubWorkflowUsesLineNumber(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way we could re-use the existing TestGithubWorkflowPinning instead of creating a new TestGitHubWorkflowUsesLineNumber ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally think it's better having it in its own test because I prefer unit tests to focus on just one thing as opposed to several disparate things. Combining it with TestGithubWorkflowPinning would make it a bit too convoluted.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @chrismcgehee

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough.

@laurentsimon laurentsimon self-requested a review August 10, 2021 15:57
@laurentsimon
Copy link
Contributor

FYI, we can start migrating to using new structure. Example in #883

@naveensrinivasan
Copy link
Member

@laurentsimon Is this good to merge?

@laurentsimon
Copy link
Contributor

laurentsimon commented Aug 24, 2021

I wanted to wait for @chrismcgehee to migrate the changes he made to the new structured results (#883), if possible.
@chrismcgehee what option do you prefer? Merge as-is or wait for you to update?

@ristomcgehee
Copy link
Contributor Author

@chrismcgehee what option do you prefer? Merge as-is or wait for you to update?

I'd like to update this PR to take advantage of the logging structure.

@laurentsimon laurentsimon enabled auto-merge (squash) August 30, 2021 16:52
@laurentsimon laurentsimon merged commit dbb2345 into ossf:main Aug 30, 2021
@ristomcgehee ristomcgehee deleted the unpinned-line-number-uses branch August 31, 2021 02:26
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.

None yet

3 participants