-
Notifications
You must be signed in to change notification settings - Fork 2
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
Support to uTCA IOC #62
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.
Please, follow the pattern for commit header we use, that is:
base: add iocStats module.
base: add ipmiComm module.
We use base:
for commits affecting the base image and ioc:
for commits affecting the IOC images. We can always git log
the file and see the past messages touching it.
Moreover, for each commit, add a item in the CHANGES.md
file under the Unreleased section following the current pattern.
35ce1fb
to
9a7e6ca
Compare
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.
We're almost there, I think.
Typo in the 9a7e6ca: s/The patches solves/The patch solves
.
9a7e6ca
to
292a572
Compare
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.
Looks fine by me. Adding @ericonr as reviewer for a second look. You may wait for his review before resolving the following point I noticed only now.
* base: add IPMIComm module. by @gustavosr8 in | ||
https://github.com/cnpem/epics-in-docker/pull/62 |
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.
Nit: I'd rather have a single line in the change list, but that's not mandatory.
292a572
to
adfb47e
Compare
adfb47e
to
33e9dca
Compare
33e9dca
to
02ca34f
Compare
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.
It looks good to me. Thanks for working on this.
Just to be sure. Have you tested this latest version with https://github.com/lnls-dig/ipmi-mgr-epics-ioc?
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 commit message has "patche" instead of "patch".
The patch solves an overflow bug and a problem when linking. Both can be found in pull requests in ipmiComm repo, and the patch can be removed when the PRs are accepted.
02ca34f
to
ca64c72
Compare
No description provided.