Skip to content

Conversation

@pdubroy
Copy link
Contributor

@pdubroy pdubroy commented Jan 28, 2022

Context

For RFC 551 APPROVED: Documenting ownership with OWNERS files, we are adding the ability to specify OWNERS of certain files/directories in our code base. We want this to behave in basically the exact same way as CODENOTIFY. However, being listed in CODENOTIFY doesn't necessarily mean you are an owner, so we need to keep the two lists separate.

As discussed in OWNERS in Sourcegraph: Implementation details, there aren't any off-the-shelf solutions that would do exactly what we want. In fact, Codenotify is the closest thing — so I decided to modify it slightly so that it can read OWNERS files as well as CODENOTIFY files.

This has the advantage that we would only have one tool and one file format to support for these two very similar purposes.

Changes in this PR

  • Add a -filename option to the codenotify tool, with a default value of "CODENOTIFY".
  • Modify the Markdown output slightly to account for the possibility of different file names.
  • Add an input to the GitHub workflow, which allows users of the workflow to pass in a different value for the filename.

@pdubroy pdubroy force-pushed the pdubroy-support-owners branch from 1c05120 to 09b43f5 Compare January 28, 2022 11:23
@pdubroy pdubroy requested a review from jhchabran January 28, 2022 11:23
@pdubroy
Copy link
Contributor Author

pdubroy commented Jan 28, 2022

@jhchabran Please take a look for early feedback. There's something not quite right about how I'm passing the parameter through from the GitHub action, but I'd like to get your feedback on the other aspects while I continue to debug that. Thanks!

@pdubroy pdubroy force-pushed the pdubroy-support-owners branch from 7bc4786 to 104fda0 Compare January 28, 2022 13:37
@sourcegraph-bot
Copy link

sourcegraph-bot commented Jan 28, 2022

Codenotify: Notifying subscribers in CODENOTIFY files for diff 515c85d...493cbc5.

Notify File(s)
@nicksnyder action.yml
main.go
main_test.go

this should be it🤞
@pdubroy pdubroy force-pushed the pdubroy-support-owners branch from 9adcce6 to 493cbc5 Compare January 28, 2022 14:36
@pdubroy pdubroy requested a review from bobheadxi January 28, 2022 14:38
Copy link

@jhchabran jhchabran left a comment

Choose a reason for hiding this comment

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

Nice 👍!

I added some comments, which provide two possible ways to simplify the logic around passing the filename from GitHub actions.

I tend to prefer the first one (where we are using the binary as the entry point and using CMD to pass additional args to overrides the defaults, but I don't have a strong opinion either.

Copy link
Member

@bobheadxi bobheadxi left a comment

Choose a reason for hiding this comment

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

I like this approach! Code LGTM as well :)

@nicksnyder
Copy link
Contributor

Happy to see you all running with this! This codebase was a bit of a hack project for me so feel free to take it from here and change whatever you want. I'll stay out of the code reviews unless you explicitly tag me :)

@pdubroy pdubroy merged commit 1a31067 into main Jan 31, 2022
@pdubroy pdubroy deleted the pdubroy-support-owners branch January 31, 2022 08:56
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.

6 participants