-
Notifications
You must be signed in to change notification settings - Fork 70
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
enable workflow at organization level #88
Conversation
Thanks for the quick turnaround here @rohankh532! Looks good overall, will take another look next week. Please include documentation for the tool in this PR (things like how to use the tool, required scopes for the PAT etc.). Could add to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rohankh532 this looks pretty good.
Just fyi, we tend to comment a lot in general, this is entirely normal ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without tests, it is hard to maintain things. For example that is the reason we are porting the shell script to go
#87
We shouldn't merge these changes without tests and high code coverage.
That is one of the reasons I am blocking this change.
@naveensrinivasan do you have any suggestions on how to unit test this? Since the code is mostly API calls to GH, it's difficult to write unit tests for it. |
I agree unit tests could be hard for this. But we should be able to add |
Makes sense. Since that is a big chunk of work in itself, can we tackle that as part of a separate PR? Note that, this PR is just a v0 version of the tool, something to get the framework in place. It will get refined more as we go before we actually recommend this to users. |
It could be a separate PR. But the issue is that no one takes the responsibility and it falls off the radar till things go wrong. That is the reason we should insist on tests for PRs. Also writing tests make the code more manageable into functions instead of a giant main func. Thoughts? |
Thinking about this more - it won't be that easy writing e2e tests also for this. Every e2e test will end up creating a bunch of new PRs and unlike Scorecards, we won't have data that is being returned to verify against. I do genuinely think writing tests for this is far more complicated than what we want to take on in this initial PR. If you still have concerns, let's discuss this in the upcoming Scorecard sync. |
I looked into writing tests and there seems to be a good example here, but it still requires a live Github account to make the changes on. Another option used here is just writing endpoints to mimic github api calls, which works well for GET requests, but I'm not sure how that would work for POSTs where we want persistence after modifications. So overall testing this seems pretty difficult, but if there was a live test Github account then it would be much easier. |
I understand! Thanks for trying and appreciate it! |
Thanks! I am ok without tests as it makes it extremely complicated. Can we please prioritize to make it small improvements to test? How about creating an issue to track it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Try to clean up the comments by starting them with a capital latter and ending them with a full stop. Also clean global variable names following golang guidelines
Ready for review. Issues to propagate to the next PR: |
@laurentsimon Is this good to be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just update one of the global variable to be lower case, and we're good to merge!
* enable workflow at organization level * added more error checking * added README, minor fixes * added support for specifying repo list * skip repo checks, started writing test * dynamically pull latest workflow file * cleanup * test file resources * reverted to statically storing workflow file * removed token * updated readme * skip repo upon failure instead of exiting * renamed global var Co-authored-by: Naveen <172697+naveensrinivasan@users.noreply.github.com>
#52