-
Notifications
You must be signed in to change notification settings - Fork 179
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
Allow substitution of user-defined variables in RPM preamble #787
Conversation
It's desirable to be able to parameterize some variables in the preamble such as architecture when RPM packages. This change enables variable substitution in the preamble section so that the values may be injected in this fashion in lieu of only using statically defined values.
Currently we don't handle things like $(foo or (bar) correctly. Lacking regex matching, we can compensate for this somewhat by attempting to find matching pairs of $( and ) and failing if we see the start of a variable declaration but not its termination.
b5f46be
to
d8c35ba
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.
Overall, the changes look reasonable to me. Could you ping me when CI is green?
Will do! |
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 PR description doesn't seem to match the code at all. It looks like the code just adds some safety against someone mis-typing a variable like $(foo} something (bar) .
The initial PR + first commit match up. When the CI job ran to test the initial changes, it hit the former case that the second commit fixes. |
It's desirable to be able to parameterize some variables in the preamble such as architecture when RPM packages. This change enables variable substitution in the preamble section so that the values may be injected in this fashion in lieu of only using statically defined values.