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: return non-zero if there were errors in processing #81

Merged
merged 4 commits into from
Sep 29, 2021

Conversation

renaissanceGeek
Copy link
Contributor

Fixes #80 very simply. There may be something more elegant, but at least this helped my use case!

Copy link
Member

@pattacini pattacini 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 the simple logic @renaissanceGeek 👍🏻
I'm just wondering if you're willing to deal also with the error conditions caught inside add_repo_collaborator that now won't make the job exit 1.

You may turn has_errors into a global var (e.g., $has_errors) as $org to handle those 3 further conditions.

Instead, a better way would be to make add_repo_collaborator return false when one of those 3 exceptions occurs so that you'll be able to account for that with has_errors locally to the main.

Just a note for myself

Since the automation may take quite some time to complete, it is convenient not to stop it early in presence of errors/warnings – which usually are not blocking – so that the specific errors can be accounted for even manually soon afterward.

Differently from this logic, the script check-automated-repositories shall exit as soon as an exception gets found as it is done right now so that the PR check can provide immediate feedback.

scripts/outside-collaborators-handler.rb Outdated Show resolved Hide resolved
@renaissanceGeek
Copy link
Contributor Author

renaissanceGeek commented Sep 28, 2021

OK, I think I resolved your requests. I tried testing various error conditions that could come up in add_repo_collaborator and successfully failed :-)

Copy link
Member

@pattacini pattacini left a comment

Choose a reason for hiding this comment

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

Fantastic job @renaissanceGeek 🚀
I've only added a missing return true and dealt with just another call to the function.

Thanks again for your contribution!
Appreciated.

@pattacini pattacini merged commit 45e8e3e into icub-tech-iit:master Sep 29, 2021
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.

Github action still seems successful even if collaborators weren't added
2 participants