-
Notifications
You must be signed in to change notification settings - Fork 21
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
Pipeline and per project configuration support #8
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.
@butchyyyy: Thanks a lot for working on this! Sorry for the late review, I've been awfully busy with school and everything, and I am barely keeping up with my work backlog! Anyway, I have a couple of comments:
-
First of all, I would recommend separating this PR into multiple commits. This PR constitutes a massive change to this plugin and would be much easier to review/merge if I could view every particular change separately. For example, I'd have a separate commit for configuring a topic to send messages to and a separate commit for configuring a stream and topic per project, and so on.
-
Also, I'd recommend structuring the commit messages like so:
jenkins: Support configuring topic per project.
You can learn more about our commit guidelines here. -
I would also recommend reading our writing guidelines for documenting integrations and seeing if we can make sure we conform to that in the
README
as much as possible. Also, it might be worth it to submit a PR on the zulip/zulip repo updating the Jenkins integration docs as well!
If you have any questions, feel free to message me on chat.zulip.org
! Thanks again!
-Eeshan
@butchyyyy: This looks much better! I took a close look at all of the commits and everything LGTM! I do think the documentation (README) may need some improvements here and there but let's do that in another PR after this one gets merged! :) Thanks for working on this! |
@eeshangarg: Thanks a lot for taking the time to look at this. Looking back at the README i noticed some typos and i'm sure there are other improvements to be made since i'm not an native english speaker. I'll be happy to improve it once this is merged. |
Merged! @butchyyyy huge thanks for your work on this. I really appreciate your taking the time to do a high-quality cleanup of this plugin! |
It is probably worth doing a round of triage together on the remaining open issues and PRs for this project, just to check which of them you've already fixed. |
PR contains several improvements to the plugin: