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 #27187 - Fixing Content View version Export for default organiz… #656

Merged
merged 1 commit into from Jul 2, 2019

Conversation

vision2910
Copy link
Contributor

Fixed the content-view version export for default organization view by putting a check to ensure no nil library instance for the repository is passed through while checking the repo download policy.

@theforeman-bot
Copy link

Issues: #27187

@theforeman-bot
Copy link

There were the following issues with the commit message:

  • length of the first commit message line for a70a2a5 exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@vision2910
Copy link
Contributor Author

vision2910 commented Jun 30, 2019

stumbled upon https://projects.theforeman.org/issues/26952 and learned that exporting the default organization view is unsupported and should be disallowed to prevent user confusion in the future. In that case, the provided PR becomes invalid.

@chris1984
Copy link
Member

chris1984 commented Jul 1, 2019

@vision2910 it is on the backlog, we need to get it done at some point, so right now we just want to error out until we get the code added. The issue is open for taking :) Otherwise, I will get to when I get some time.

I will test this and see if it works the way we want to and reply back.

@chris1984 chris1984 self-assigned this Jul 1, 2019
Copy link
Member

@akofink akofink left a comment

Choose a reason for hiding this comment

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

Seems fine - what do you think about adding a test for this case?

@vision2910
Copy link
Contributor Author

raised a PR for https://projects.theforeman.org/issues/26952.

@chris1984 @akofink sounds like a good idea for testing this PR's functionality. I will add a test for this case.

@mccun934
Copy link
Member

mccun934 commented Jul 2, 2019

I tested this, works fine, exported the default org view without issues.

@chris1984
Copy link
Member

Thanks @vision2910

@chris1984 chris1984 merged commit ebf4e68 into Katello:master Jul 2, 2019
@vision2910
Copy link
Contributor Author

Thanks @mccun934, @chris1984, @akofink

@akofink
Copy link
Member

akofink commented Jul 3, 2019

Merged without tests? :( 🙈

@chris1984
Copy link
Member

I will make another pr with updated tests me-- :(

@vision2910
Copy link
Contributor Author

I will make another pr with updated tests me-- :(

added the test #659

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants