Skip to content
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

Issues: #1116 - Multi-step fix for RPM and DEB package validation #1117

Merged

Conversation

ssorenso
Copy link

Fixes #1116

Problem:
This issue is a few fold:

  1. RPM spec file format was not being followed properly
  • Requires: , [repeat]
  1. RPM was not actually installing dependencies during test
  2. RPM was not failing when project package failed to install during test
  3. DEB was not actually installing dependencies during test
  4. DEB was not fialing when project package failed to install during test

Analysis:

  1. added a check_requires() function that will add the proper formatting
  • Change performed in the build-rpm.py
  1. Changed the Dependency.cmd to be handled differently with a
    Dependency.install_cmd
  • Change performed in the redhat's fetch_and_install_deps.py
  1. Fix was multi-fold, but all under redhat's fetch_and_install_deps.py:
  • runCommand() was changed to use subprocess.check_output
  • runCommand()'s call for rpm -i now has a status check
  • a last verification against rpm -qa is performed
  1. Made a Dependency._install_req to be cond. exec. by parent
  • This is always executed by F5Dependency
  • This is added to the ubuntu's fetch_and_install_deps.py
  1. Again multi-fold in the ubuntu's fetch_and_install_deps.py:
  • runCommand() was changed to use subprocess.check_output
  • runCommand()'s call for dpkg -i now has a status check
  • a last verification against dpkg -l is performed

Tests:
This is a test in and of itself; however, there is now an additional
ring of tests for both ubuntu and debian that assures, using its
individual packaging medium, a means to verify that the package is, in
fact, installed after the processing of the test scripts on the
associated docker container instance.

Assignee:
@pjbreaux

Reviewers:
@richbrowne

@ssorenso ssorenso requested a review from pjbreaux April 13, 2017 22:37
@ssorenso ssorenso assigned ssorenso and pjbreaux and unassigned ssorenso Apr 13, 2017
@ssorenso ssorenso requested a review from richbrowne April 13, 2017 22:38
@ssorenso ssorenso force-pushed the feature.packaging_error_1116 branch 2 times, most recently from 3fd2219 to bff5816 Compare April 13, 2017 22:58
match = breakout_re.search(requirement)
if match:
req = Req(*match.groups())
mod = 'python-' + req.module \
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to assume that we'll always use a library with python- pre-appended.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the pattern followed by RHEL. If we are depending on python libs outside of the set of RHEL supported packages, the we have to ask "Do we really need them?"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks!

runCommand('rpm -i %s' % tmp_pkg_name)
output, result = runCommand('rpm -i %s 2>&1' % tmp_pkg_name)
if not result == 0:
raise InstallError("Exist status was {}".format(result))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exist should be Exit

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a s/Exist/Exit/

@ssorenso ssorenso force-pushed the feature.packaging_error_1116 branch from bff5816 to f508d9f Compare April 14, 2017 21:18
@@ -447,7 +454,12 @@ def fetch_pkg_dependencies(config, pkg_name):

print("Installing Self - %s" % pkg_name)
try:
runCommand('dpkg -i %s' % tmp_pkg_name)
output, result = runCommand('dpkg -l | grep f5-')
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the -l call.

Fixes F5Networks#1116

Problem:
This issue is a few fold:
1. RPM spec file format was not being followed properly
  * Requires: <module> <modifier> <version>, [repeat]
2. RPM was not actually installing dependencies during test
3. RPM was not failing when project package failed to install during test
4. DEB was not actually installing dependencies during test
5. DEB was not fialing when project package failed to install during test

Analysis:
1. added a check_requires() function that will add the proper formatting
  * Change performed in the build-rpm.py
2. Changed the Dependency.cmd to be handled differently with a
Dependency.install_cmd
  * Change performed in the redhat's fetch_and_install_deps.py
3. Fix was multi-fold, but all under redhat's fetch_and_install_deps.py:
  * runCommand() was changed to use subprocess.check_output
  * runCommand()'s call for rpm -i <project> now has a status check
  * a last verification against rpm -qa is performed
4. Made a Dependency._install_req to be cond. exec. by parent
  * This is always executed by F5Dependency
  * This is added to the ubuntu's fetch_and_install_deps.py
5. Again multi-fold in the ubuntu's fetch_and_install_deps.py:
  * runCommand() was changed to use subprocess.check_output
  * runCommand()'s call for dpkg -i <project> now has a status check
  * a last verification against dpkg -l is performed

Tests:
This is a test in and of itself; however, there is now an additional
ring of tests for both ubuntu and debian that assures, using its
individual packaging medium, a means to verify that the package is, in
fact, installed after the processing of the test scripts on the
associated docker container instance.
@ssorenso ssorenso force-pushed the feature.packaging_error_1116 branch from f508d9f to 82c2f47 Compare April 19, 2017 15:40
Copy link
Contributor

@pjbreaux pjbreaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this fix is ready to go.

@ssorenso
Copy link
Author

@richbrowne, any chance of a check for this? Just trying to clean up the dots on my radar screen...

Thanks

@wojtek0806
Copy link
Contributor

seeing this was approved i am merging this

@wojtek0806 wojtek0806 merged commit 6a555e6 into F5Networks:development May 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants