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

Refs #22620 - make UI honor enabled repo types #7204

Merged
merged 1 commit into from Feb 26, 2018

Conversation

jlsherrill
Copy link
Member

This adds more awareness around what repo types are
enabled, on the lifecycle env page, the main navigation,
and updates detection on the new repo page

@theforeman-bot
Copy link

Issues: #22620

@jlsherrill
Copy link
Member Author

requires Katello/bastion#223

@akofink
Copy link
Member

akofink commented Feb 20, 2018

Getting the same error locally as the CI:

NameError: uninitialized constant Katello::RepositoryTypes

Is this PR WIP?

@jlsherrill
Copy link
Member Author

@akofink sorry, had a mistake, updated.

@akofink
Copy link
Member

akofink commented Feb 21, 2018

Hrm... now, when trying to create a new repository, the 'Type' dropdown list is empty. There are no JS errors. Are you seeing this?

@jlsherrill
Copy link
Member Author

@akofink sorry about that, updated.

akofink
akofink previously approved these changes Feb 22, 2018
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.

APT! It now works as expected.

@jlsherrill
Copy link
Member Author

@akofink please retest this. This did a couple of things:

Fixed the global nav, it wasn't hiding nav items based on enabled repo types
adding controlling of enabled content types via a configuration (katello.yml) see updates to the example

@akofink akofink dismissed their stale review February 23, 2018 16:22

code changes

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.

Works as expected. A bit of IRC discussion:

  akofink | I have all the content types turned off :)
jsherrill | hehe
  akofink | I like how it still shows me content that I synced, even though it's turned  off
        ⤷ | That's intentional, right? On the lifecycle environments page
        ⤷ | seems so from the code
jsherrill | akofink: yeah
        ⤷ | when possible we try to just go off what you've synced
        ⤷ | so the ui gets easier to use the fewer types of content you've synced
        ⤷ | the content view versions page does this already
        ⤷ | content view version details i mean
  akofink | but it can be a bit weird because I can see my existing yum repos, but I can't make any new ones :)
jsherrill | yeah
        ⤷ | normally you'd just define what types you want at install time
        ⤷ | (PRs to come)
        ⤷ | and not turn them off willy-nilly
  akofink | It seems like a use case that users wouldn't get into. If they wanted to stop using yum, step 1 would be delete all the yum repos
        ⤷ | then disable it
        ⤷ | jsherrill: what I don't like is, I can still sync my yum repo successfully
jsherrill | akofink: well the goal is really to be able to install katello without support for one or more content types
  akofink | jsherrill: I'm fine with it as is, if you think the use case will be to disable from installation. Just know that someone somewhere will do what I just did and have questions.
          | ;)
jsherrill | yeah, its good to know the limitations :)
        ⤷ | and potential questions

:deb: true
:puppet: true
:docker: true
:ostree: true
Copy link
Member

Choose a reason for hiding this comment

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

I think you should put some comments. Namely that the default is that all registered content types are allowed. Optionally, that the Katello repository types are located and registered from /lib/katello/repository_types/*.rb.

Copy link
Member

@akofink akofink Feb 26, 2018

Choose a reason for hiding this comment

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

I mostly wanted a comment saying that the default is enabled: true if content_types array is empty/nonexistent

@jlsherrill
Copy link
Member Author

@akofink added

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.

Thanks @jlsherrill!

This adds more awareness around what repo types are
enabled, on the lifecycle env page, the main navigation,
and updates detection on the new repo page
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants