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

Fixes #13585 - Adding Ostree functionality #5755

Merged
merged 1 commit into from Feb 15, 2016

Conversation

Projects
None yet
6 participants
@parthaa
Copy link
Member

commented Feb 4, 2016

Squashing every thing in the OSTree branch into 1 huge commit.

Stories covered by this PR

Fixes #10042 - Enable ostree repos in the CDN -
#5455

Fixes #11611 - Copy over ostree branches on publish and promote -
#5449

Ref #10040 - Minor fixes for the ostree repo create code -
#5448

Fixes #10066 - Support promoting ostree repos -
#5447

Refs #10040 - Ostree branch change now updates pulp -
#5431

Fixes #10063 - Allow publishing of ostree repos in content views -
#5415

Fixes #11567 - Fix relative_path for ostree repos -
#5442

Fixes #10040 - UI Bindings to CRUD rpm-ostree -
#5394
Includes adding/updating/removing branches

Fixes #10044 - UI to remove/add ostree repos to CVs -
#5361
Fixes #10056 - Adding ostree sync -
#5306
Refs #10062 - Allow users to add/remove ostree repos - #5337

Refs #10055 - Initial model bindings for OSTREE CRUD -
#5240

@daviddavis

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

👍

@parthaa parthaa force-pushed the parthaa:new-ostress branch 2 times, most recently Feb 4, 2016

@ehelms

This comment has been minimized.

Copy link
Member

commented Feb 5, 2016

Thanks for pulling off the squash, that is much appreciated! Given the age of the branch this came from, I would like to test it and do a high level review.

  • Can you point me at or add a comment with what is needed to test this?
  • This currently points at the OSTree tracker, which would close the tracker when merged. However, there are still issues marked against the tracker. I would recommend either a new issue that encompasses this marked against the tracker or updating this to 'Refs'. This will help with tracking during release time.
# TODO
# add a hard coded config entry branch for now
# we will add pull all once the summary files functionality is available
repository.ostree_branches.new(:name => Setting[:default_cdn_ostree_branch_name], :repository => repository)

This comment has been minimized.

Copy link
@ehelms

ehelms Feb 5, 2016

Member

Saw this comment and wanted to ask about it given this was written a while back and Pulp 2.8 has summary support.

This comment has been minimized.

Copy link
@parthaa

parthaa Feb 5, 2016

Author Member

I thoughI I created an issue for that ...

This comment has been minimized.

This comment has been minimized.

Copy link
@parthaa

parthaa Feb 5, 2016

Author Member

@ehelms this would be a good follow up PR.

@ehelms

This comment has been minimized.

Copy link
Member

commented Feb 5, 2016

You will also want to go ahead and open a PR to enable OStree during installation -- see https://github.com/Katello/puppet-katello/blob/master/manifests/init.pp#L116-L118

@parthaa parthaa force-pushed the parthaa:new-ostress branch 3 times, most recently Feb 5, 2016

@parthaa parthaa changed the title Fixes #10034 - Adding Ostree functionality Fixes #13585 - Adding Ostree functionality Feb 5, 2016

@parthaa

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2016

Thanks for pulling off the squash, that is much appreciated! Given the age of the branch this came from, I would like to test it and do a high level review.

Can you point me at or add a comment with what is needed to test this?

Note every thing in this PR has already been reviewed and acked. Its been through the same grind as a regular PR codewise. That being said we might want to re review the functionality atleast considering it was written for Pulp 2.7 and master now has 2.8.

To test this branch you need to have ostree/pulp-ostree-plugins installed. On my centos 7 box I have the following repo enabled to yum install -y ostree rpm-ostree

[atomic7-testing]
name=atomic7-testing
baseurl=http://cbs.centos.org/repos/atomic7-testing/x86_64/os/
gpgcheck=0
enabled=1

Unfortunately ostree doesn't seem to be in the base yet. We'd need to figure some thing out for rhel also. => http://cbs.centos.org/koji/buildinfo?buildID=1490 only points to atomic7-testing ...

High level stories you can test

  • UI/API - As a user I want to be able to CRUD an rpm-ostree. Same repo create UI. A good value fo the feed -> https://dl.fedoraproject.org/pub/fedora/linux/atomic/22/
  • UI/API - As a user I want to be able to add/update ostree branches. There is now a "Manage Branches" in the repo details that lets you add remove branches. Right now the summary is not automatically populated. Going to be fixed in a future stories (http://projects.theforeman.org/issues/13586, http://projects.theforeman.org/issues/13587, http://projects.theforeman.org/issues/13588 ). You need figure out the available branches by following the "refs/heads//....". The branch name will be stuff that comes after refs/heads. For example a good value for the branch for the fedora project feed -> fedora-atomic/f22/x86_64/docker-host (considering this as the navigation path https://dl.fedoraproject.org/pub/fedora/linux/atomic/22/refs/heads/fedora-atomic/f22/x86_64/docker-host)
  • UI/API - As a user I want to be able to sync an rpm-ostree repo -> Same as present. Check whether contents are available at /var/lib/pulp/published/ostree/web/ after sync. You should also be able to navigate to -> https://<katello fqdn>/pulp/ostree/web/Default_Organization/Library/custom/<product>/<repo>/...
  • UI/API - As a user I want to be able to enable an rpm-ostree repo via CDN manifest - There should be an ostree tab now and should let you enable/disable repos
  • UI/API - As a user I want to be able to import a manifest - Same as regular (make sure manifest has atomic stuff)
  • UI/API - As a user I want to be able to sync a CDN based rpm-ostree repo - Same enable + sync process as any other repo.
  • UI/API - As a user I want to be able to add/remove rpm-ostree repos to a content view - There should be a OSTree tab where you can add/remove ostree repos.
  • UI/API - As a user I want to be able to publish rpm-ostree repos to a content view -> Same as regular
  • UI/API - As a user I want to be able to promote rpm-ostree repos to a content view -> Same as regular

This currently points at the OSTree tracker, which would close the tracker when merged. However, there are still issues marked against the tracker. I would recommend either a new issue that encompasses this marked against the tracker or updating this to 'Refs'. This will help with tracking during release time.

I created a new issue. Tracker will close when other issues including summary-files content-host handling are addressed.

For content host handling we need this PR addressed

theforeman/puppet-certs#67

@beav

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

👍

@parthaa parthaa force-pushed the parthaa:new-ostress branch Feb 11, 2016

@parthaa

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

You will also want to go ahead and open a PR to enable OStree during installation -- see https://github.com/Katello/puppet-katello/blob/master/manifests/init.pp#L116-L118

http://projects.theforeman.org/issues/13626

@parthaa

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

You will also want to go ahead and open a PR to enable OStree during installation -- see https://github.com/Katello/puppet-katello/blob/master/manifests/init.pp#L116-L118

also
http://projects.theforeman.org/issues/13625

@ehelms

This comment has been minimized.

Copy link
Member

commented Feb 12, 2016

I created and synced the repository you mentioned. I am showing no 'OStree Units' and no repository created at the published URL presented in the UI to me.

@parthaa parthaa force-pushed the parthaa:new-ostress branch 2 times, most recently Feb 12, 2016

@bbuckingham

This comment has been minimized.

Copy link
Member

commented Feb 12, 2016

@parthaa, I reviewed the code and do not see any glaring issues; however, as you mention it has been through previous reviews. I also ran some tests with the PR and observed the same behaviors as @ehelms above wrt to not seeing units on the repository. This makes it a bit difficult to validate managing the repo as well as seeing the outcome of content view publishes and promotions (which do appear to work).

Fixes #13585 - Adding Ostree functionality
Changes include

Fixes #10042 - Enable ostree repos in the CDN -
#5455

Fixes #11611 - Copy over ostree branches on publish and promote -
#5449

Ref #10040 - Minor fixes for the ostree repo create code -
#5448

1) Added the missing auto_publish : true setting for OSTree repo
creation
2) Removed the nodes distributor for ostree since we are changing that
mechanism to a different model. (Nodes distributor is also not supported
for ostree content.)

Fixes #10066 - Support promoting ostree repos -
#5447

Refs #10040 - Ostree branch change now updates pulp -
#5431

Fixes #10063 - Allow publishing of ostree repos in content views -
#5415

Fixes #11567 - Fix relative_path for ostree repos -
#5442

Fixes #10040 - UI Bindings to CRUD rpm-ostree -
#5394
Includes adding/updating/removing  branches

Fixes #10044 - UI to remove/add ostree repos to CVs -
#5361
Fixes #10056 - Adding ostree sync -
#5306
Refs #10062 - Allow users to add/remove ostree repos
-#5337

Refs #10055 - Initial model bindings for OSTREE CRUD -
#5240

@parthaa parthaa force-pushed the parthaa:new-ostress branch to 69e1531 Feb 12, 2016

@parthaa

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2016

@bbuckingham @ehelms http://projects.theforeman.org/issues/11612 <- OSTree content unit count not updating .

@parthaa

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2016

This makes it a bit difficult to validate managing the repo as well as seeing the outcome of content view publishes and promotions (which do appear to work).

So there are 2 ways to validate it

  1. Make sure content is accessible https:///pulp/ostree/web/Default_Organization/Library/custom/// works. Note the https part is important here. Similar structure for CVE's
  2. Associate this to an activation key and use subscription manager from an atomic rhel consumer. For a non rhel consumer like fedora/centos you'll have to tweak file. I 'll have instructions for that in a bit..
@parthaa

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2016

I created and synced the repository you mentioned. No repository created at the published URL presented in the UI to me.

I created an issue to show the right published at URL. http://projects.theforeman.org/issues/13695
Use https://<katello fqdn>/pulp/ostree/web/<org>/Library/custom/<product>/<repo>/ instead of http://<katello fqdn>/pulp/repos/<org>/Library/custom/<product>/<repo>/

I am filing new issues because I do not want to tamper the existing reviewed code unless absolutely necessary.

@beav

This comment has been minimized.

Copy link
Member

commented Feb 14, 2016

If the goal is to avoid additional change, I say just merge this PR and fix any issues as new PRs on master since we are treating issues as separate bugs anyway.

That is my $0.02 but I'd like to get other opinions 🐨

@bbuckingham

This comment has been minimized.

Copy link
Member

commented Feb 14, 2016

@beav, I agree. If no additional changes are going to be made and the changes so far do not affect any existing features, we should get this merged in to master. That would allow the feature to move forward as well as more easily allow additional developers to help contribute to completion of any remaining redmines. That said, I'll give it an ACK, but additional reviewers should probably also ACK.

@ehelms

This comment has been minimized.

Copy link
Member

commented Feb 15, 2016

Missing features or enhancements that are nice to have I am usually OK with merging, when there are obviously broken aspects that is tougher for me to knowingly merge in to our codebase. Given they are not egregious I can let them slide personally, but lets not make this a habit. Are there open issues for showing ostree content on content view version details and lifecycle environment pages?

@parthaa

This comment has been minimized.

@ehelms

This comment has been minimized.

Copy link
Member

commented Feb 15, 2016

ACK

parthaa added a commit that referenced this pull request Feb 15, 2016

Merge pull request #5755 from parthaa/new-ostress
Fixes #13585 - Adding Ostree functionality

@parthaa parthaa merged commit 670aa46 into Katello:master Feb 15, 2016

1 check passed

default Job result: SUCCESS
Details

@parthaa parthaa deleted the parthaa:new-ostress branch Feb 15, 2016

@daviddavis

This comment has been minimized.

Copy link
Member

commented Feb 16, 2016

giphy

@beav

This comment has been minimized.

Copy link
Member

commented Feb 16, 2016

🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.