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

Initial refactoring of yumrepo. #2086

Merged
merged 7 commits into from Feb 14, 2014

Conversation

Projects
None yet
8 participants
@apenney
Contributor

apenney commented Nov 18, 2013

[I've made this PR early, as I'd like feedback on the direction of the provider/type, and the tests. It seems to work for me, which is about as strongly tested as it is right now.]

This work strips out all of the provider code from the type, and
creates a new ruby provider for yumrepo.

While this code still uses inifile it's been rewritten to take advantage
of the modernization of Puppet. It's now a little easier to understand
and test.

@puppetcla

This comment has been minimized.

Show comment
Hide comment
@puppetcla

puppetcla Nov 19, 2013

CLA signed by all contributors.

puppetcla commented Nov 19, 2013

CLA signed by all contributors.

@Sharpie

This comment has been minimized.

Show comment
Hide comment
@Sharpie

Sharpie Nov 27, 2013

Member

Related tickets:

Redmine 8758
Redmine 9293

Member

Sharpie commented Nov 27, 2013

Related tickets:

Redmine 8758
Redmine 9293

@Sharpie

This comment has been minimized.

Show comment
Hide comment
@Sharpie

Sharpie Nov 27, 2013

Member

Also incorporates Redmine 22304

Member

Sharpie commented Nov 27, 2013

Also incorporates Redmine 22304

@@ -0,0 +1,149 @@
require 'puppet/util/inifile'
Puppet::Type.type(:yumrepo).provide(:inifile) do

This comment has been minimized.

@apenney

apenney Jan 22, 2014

Contributor

Renamed to inifile from ruby here.

@apenney

apenney Jan 22, 2014

Contributor

Renamed to inifile from ruby here.

@unux

This comment has been minimized.

Show comment
Hide comment
@unux

unux Jan 22, 2014

@apenney from the yum config parser: http://yum.baseurl.org/gitweb?p=yum.git;a=blob;f=yum/config.py#l387

class BoolOption(Option):
    """An option representing a boolean value.  The value can be one
        of 0, 1, yes, no, true, or false.
        """

Seems like yum's boolean options also take yes and no in addition to (0|1|false|true). This might be worth expanding into the rest of yumrepos options.

unux commented Jan 22, 2014

@apenney from the yum config parser: http://yum.baseurl.org/gitweb?p=yum.git;a=blob;f=yum/config.py#l387

class BoolOption(Option):
    """An option representing a boolean value.  The value can be one
        of 0, 1, yes, no, true, or false.
        """

Seems like yum's boolean options also take yes and no in addition to (0|1|false|true). This might be worth expanding into the rest of yumrepos options.

@apenney

This comment has been minimized.

Show comment
Hide comment
@apenney

apenney Jan 22, 2014

Contributor

Ahhhh, nice, I didn't look in config.py, I had only got as far as repo.py. That'll teach me. I'll fix this globally.

Contributor

apenney commented Jan 22, 2014

Ahhhh, nice, I didn't look in config.py, I had only got as far as repo.py. That'll teach me. I'll fix this globally.

# We delete the file and then remove it from the list of files.
if destroy
::File.unlink(file)
@files.delete(file)

This comment has been minimized.

@adrienthebo

adrienthebo Jan 22, 2014

Contributor

This has been a longstanding bug, it would be really good to link this change to the relevant tickets and update the PR message/commits to reflect that this is fixed.

@adrienthebo

adrienthebo Jan 22, 2014

Contributor

This has been a longstanding bug, it would be really good to link this change to the relevant tickets and update the PR message/commits to reflect that this is fixed.

@apenney

This comment has been minimized.

Show comment
Hide comment
@apenney

apenney Jan 28, 2014

Contributor

I'll keep working on this. I'll make sure self.reposdir is covered as well.

Contributor

apenney commented Jan 28, 2014

I'll keep working on this. I'll make sure self.reposdir is covered as well.

@riton

This comment has been minimized.

Show comment
Hide comment
@riton

riton Jan 28, 2014

Contributor

Hey, for #2268, you can now pull from 4599332 il you're okay with this.

Cheers

Contributor

riton commented Jan 28, 2014

Hey, for #2268, you can now pull from 4599332 il you're okay with this.

Cheers

@joshcooper

This comment has been minimized.

Show comment
Hide comment
@joshcooper

joshcooper Jan 29, 2014

Member

@apenney I noticed that travis failed on commits 866f5c5 and 537b6f4. Could you fix those up? That way it's easier to bisect in the future.

Member

joshcooper commented Jan 29, 2014

@apenney I noticed that travis failed on commits 866f5c5 and 537b6f4. Could you fix those up? That way it's easier to bisect in the future.

Ashley Penney
(PUP-1066) Add the ability to delete files.
Add a .destory and .destroy? in order to flag files for deletion.
We also sneak in a .entries reader in order to let us read the
entries directly.

This should fix PUP-1066 and #12687.
end
end
describe 'create' do

This comment has been minimized.

@adrienthebo

adrienthebo Feb 4, 2014

Contributor

These methods look a lot better, nice work.

@adrienthebo

adrienthebo Feb 4, 2014

Contributor

These methods look a lot better, nice work.

Ashley Penney added some commits Nov 14, 2013

Ashley Penney
(PUP-789) Break yumrepo into a type/provider.
This work strips out all of the provider code from the type, and
creates a new ruby provider for yumrepo.

While this code still uses inifile it's been rewritten to take advantage
of the modernization of Puppet.  It's now a little easier to understand
and test.

This covers: #8758, #9293, #22304 (projects.puppetlabs.com)

adrienthebo added a commit that referenced this pull request Feb 14, 2014

Merge pull request #2086 from apenney/yumrepo
(PUP-789) refactor yumrepo into type and provider

@adrienthebo adrienthebo merged commit e638972 into puppetlabs:master Feb 14, 2014

1 check passed

default The Travis CI build passed
Details
@adrienthebo

This comment has been minimized.

Show comment
Hide comment
@adrienthebo

adrienthebo Feb 14, 2014

Contributor

Merged into master in e638972; this should be released in 3.5.0. Thanks for all your hard work!

Contributor

adrienthebo commented Feb 14, 2014

Merged into master in e638972; this should be released in 3.5.0. Thanks for all your hard work!

# @param conf [String] Configuration file to check for value.
# @return [String] The value of a looked up key from the configuration file.
def self.find_conf_value(value, conf='/etc/yum.conf')
if File.exists?(conf)

This comment has been minimized.

@kylog

kylog Feb 14, 2014

After seeing @peterhuene's fix up yesterday at a76d681, I took a look at this pull. There are some file api accesses via the native ruby api (e.g. here) and some via the Puppet::FileSystem abstraction (e.g. line 50). For consistency's sake, we should probably scrub this pull to use that abstraction consistently. @apenney @adrienthebo thoughts?

@kylog

kylog Feb 14, 2014

After seeing @peterhuene's fix up yesterday at a76d681, I took a look at this pull. There are some file api accesses via the native ruby api (e.g. here) and some via the Puppet::FileSystem abstraction (e.g. line 50). For consistency's sake, we should probably scrub this pull to use that abstraction consistently. @apenney @adrienthebo thoughts?

@adrienthebo

This comment has been minimized.

Show comment
Hide comment
@adrienthebo

adrienthebo Feb 14, 2014

Contributor

I agree, I'll go though and do the needful shortly.

Contributor

adrienthebo commented Feb 14, 2014

I agree, I'll go though and do the needful shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment