-
Notifications
You must be signed in to change notification settings - Fork 22
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
Adding Default Action #31
Conversation
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.
A few inline comments as a first pass, but looking good so far.
Thanks @leplatrem; I've resolved the comments in the most recent commit I believe. |
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.
There are a couple of things that can still be improved I think, if you still have some forces ;)
- the fact that we return 201 on errors. See my comment https://github.com/mozilla/jira-bugzilla-integration/pull/31/files#r838330014
- the fact that we refetch/parse the bug in the action code https://github.com/mozilla/jira-bugzilla-integration/pull/31/files#r838340107
- the way jira and bugzilla are mocked in tests
We've never been so close ;)
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 again @leplatrem for your review; I've added some comments--and willing to further adjust, but I think most of the comments have been resolved in the most recent commit.
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.
💯
The 202 status is fine yes. What was wondering me most was the catch all of action errors.
The only thing left is the logging. I'm a little bit scared to have requests ignored silently, especially for the first cycles of deployment.
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.
@leplatrem I went through and made some changes based on your most recent comments.
I tried to add the suggested helper as an injected method in the bugzilla client returned from the get_bugzilla call, but something seemed off with that approach.
…iders-issue Bugfix/terraform providers issue
No description provided.