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

refactor gpg renderer; removing dependency on python-gnupg #24314

Closed
wants to merge 2 commits into from

Conversation

cedwards
Copy link
Contributor

@cedwards cedwards commented Jun 1, 2015

This patch updates the gpg renderer to avoid the dependency on python-gnupg. I feel this patch is useful for two reasons:

  1. fewer package dependencies are always better.
  2. python-gnupg essentially calls out to the GPG binary anyway, so this just cuts out the middle man.

@jfindlay
Copy link
Contributor

jfindlay commented Jun 2, 2015

@cedwards, will you also look at the gpg test failures? Thanks.

@cedwards
Copy link
Contributor Author

cedwards commented Jun 2, 2015

@jfindlay - Yes, I will look into the unit tests. I'm not immediately sure why it's failing--the updated module works as expected in my infrastructure. I'll let you know what I discover.

@jfindlay jfindlay added Expert Change Core relates to code central or existential to Salt and removed Master Change labels Jun 3, 2015
@cachedout
Copy link
Contributor

@cedwards A friendly ping. Have you been able to investigate these failures? This is a great addition and we would really like to get it merged! :]

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jun 17, 2015
@cedwards
Copy link
Contributor Author

I have not yet had time to dig into the test failures. I need to be more familiar with the unit tests to understand why it's failing.

Not to be "that guy", but I'm using this patch on my production systems and it's working as expected, so I assume it's something specifically related to the unit tests expecting GPG. I'll hopefully be able to look into it soon.

@cachedout
Copy link
Contributor

Totally fine. I'd much rather you let us know versus letting this sit. I'll have a look-see tomorrow as well and we'll hopefully get this thing merged. Many thanks!

@cachedout
Copy link
Contributor

I played around with this a bit this morning. Since the test suite relies on mocking the GPG python module, we'll need to basically rewrite the GPG renderer tests. I will create a ticket for the QA folks to do so.

@cachedout
Copy link
Contributor

@jfindlay is going to write some tests for this and then we'll get it merged. Apologies for the delay here.

@jfindlay jfindlay mentioned this pull request Jul 16, 2015
thatch45 added a commit that referenced this pull request Jul 16, 2015
@jfindlay
Copy link
Contributor

#25470 has these changes and the rewritten tests, so I'm going to close this. Thanks @cedwards.

@jfindlay jfindlay closed this Jul 16, 2015
@cachedout
Copy link
Contributor

This is awesome! Thanks very much @jfindlay and @cedwards

@cedwards
Copy link
Contributor Author

@jfindlay @cachedout - I notice this merge continues to be skipped with each point release. Are we planning on including this at any point? I'd really like to remove the dependency on python-gnupg.

I'm happy to update it if needed against current develop.

@jfindlay
Copy link
Contributor

@cedwards, this has been backported in #28921.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core relates to code central or existential to Salt Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants