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

(PE-25542) add RHEL8 to puppet agent module #379

Merged
merged 1 commit into from Feb 1, 2019
Merged

(PE-25542) add RHEL8 to puppet agent module #379

merged 1 commit into from Feb 1, 2019

Conversation

loopinu
Copy link
Contributor

@loopinu loopinu commented Jan 30, 2019

added red hat 8 to metadata.json & puppet_agent_osfamily_redhad_spec.rb

Please do not merge yet.

Copy link
Contributor

@ScottGarman ScottGarman left a comment

Choose a reason for hiding this comment

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

LGTM, I'd recommend also updating the commit message to something more like what was done for Fedora 29 to follow our usual style: 1a53054

Copy link
Member

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

The current implementation of the puppet_agent::install task does not work for rhel 8. Specifically it tries to download http://yum.puppetlabs.com/puppet/puppet-release-el-8.noarch.rpm
That could be a problem with the implementation, specifically I see that http://yum.puppetlabs.com/puppet/el/8/x86_64/puppet-agent-6.2.0-1.el8.x86_64.rpm is available. Should we target that directory for installing the agent?

"el")
info "Red hat like platform! Lets get you an RPM..."
filetype="rpm"
filename="${collection}-release-el-${platform_version}.noarch.rpm"
download_url="http://yum.puppetlabs.com/${collection}/${filename}"
;;

To be clear I don't think this should block merging this. I just wanted to point out the shortcoming in the install task implementation.

@puppetcla
Copy link

CLA signed by all contributors.

Copy link
Contributor

@caseywilliams caseywilliams left a comment

Choose a reason for hiding this comment

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

Acceptance upgrade tests fail here, but only because we haven't released a beta package for RHEL 8 in the puppet5 series (which is where we'd be upgrading to puppet6 from) -- I'm ready to merge this now, but I'll wait for another reviewer.

@caseywilliams
Copy link
Contributor

@donoghuc I just saw your comment -- my understanding is that the invalid path that the task currently relies on is not the "official" download path, and that the second one you mention is correct for all platforms (although checking with Release Engineering may be helpful) -- I believe that the older paths there are kept intact for those who still rely on them (although I do find their presence confusing!)

@mwaggett
Copy link
Contributor

release packages for el 8 have now been shipped, so downloading from http://yum.puppetlabs.com/puppet/puppet-release-el-8.noarch.rpm should work

@donoghuc
Copy link
Member

Just confirmed that this now works!

@loopinu
Copy link
Contributor Author

loopinu commented Feb 1, 2019

@donoghuc , @caseywilliams Thanks for reviewing this PR and thank you @mwaggett for the release packages.

Short question: Can this be merged independently or it has a dependency with PR: https://github.com/puppetlabs/puppetlabs-pe_repo/pull/554 ?

Thanks

@ScottGarman ScottGarman merged commit c1b6498 into puppetlabs:master Feb 1, 2019
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.

None yet

7 participants