-
Notifications
You must be signed in to change notification settings - Fork 4
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
build-rpms: Use common target to build rpms #66
build-rpms: Use common target to build rpms #66
Conversation
With samba-in-kubernetes/samba-build#48 we combine version specific build targets for CentOS using a general makefile rule.
🎉 All dependencies have been resolved ! |
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.
Generally a good and logical change, but I think something is missed here. See inline.
make "test.rpms.${PLATFORM}${VERSION}" "refspec=${SAMBA_BRANCH}" | ||
fi | ||
make "rpms.${PLATFORM}" "vers=${VERSION}" "refspec=${SAMBA_BRANCH}" | ||
make "test.rpms.${PLATFORM}" "vers=${VERSION}" "refspec=${SAMBA_BRANCH}" |
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.
I was expecting to see make rpms.${PLARFORM}
instead of an if/else branching.
Doesn't keeping the if/else branch defeat the purpose of this PR and the preparatory samba-build PR?
I suggest to remove the if-else branch and only use make rpms.${PLATFORM}
directly.
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.
I was not looking right, confused removal with addition. forget this review...
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.
I withdraw my previous change request. This is exactly how it should be. - apologies!
LGTM!
make "test.rpms.${PLATFORM}${VERSION}" "refspec=${SAMBA_BRANCH}" | ||
fi | ||
make "rpms.${PLATFORM}" "vers=${VERSION}" "refspec=${SAMBA_BRANCH}" | ||
make "test.rpms.${PLATFORM}" "vers=${VERSION}" "refspec=${SAMBA_BRANCH}" |
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.
I was not looking right, confused removal with addition. forget this review...
With samba-in-kubernetes/samba-build#48 we combine version specific build targets for CentOS using a general makefile rule.
depends on samba-in-kubernetes/samba-build#48