-
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
Add support for "Epoch" attributes in RPMs #858
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.
I will wait to merge so that @aiuto can take a look.
tests/rpm/BUILD
Outdated
@@ -179,6 +179,7 @@ pkg_rpm( | |||
architecture = "noarch", | |||
conflicts = ["not-a-test"], | |||
description = """pkg_rpm test rpm description""", | |||
epoch = "1", |
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.
Since this is optional, it should not be in every test case.
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.
Agree! Updated 😄
tests/rpm/pkg_rpm_basic_test.py
Outdated
@@ -74,6 +74,7 @@ def test_scriptlet_content(self): | |||
|
|||
def test_basic_headers(self): | |||
common_fields = { | |||
"EPOCH": b"1", |
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.
Can you pass in the check for EPOCH to only the samples which add it?
I know that is not the same pattern here, but what is here was hacked up in a hurry. Having version and release the same for everything is not so bad, but we should have split out noarch and group so they can vary by test case.
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.
Done
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 for the change
Fixes #857