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

Update Limitations in README #398

Merged
merged 2 commits into from
Apr 15, 2021
Merged

Update Limitations in README #398

merged 2 commits into from
Apr 15, 2021

Conversation

moritzdietz
Copy link
Contributor

Update the supported platforms to include RHEL8 as well. Support for this was added with #335 but this change didn't update the README as well.

Closes #397

@moritzdietz
Copy link
Contributor Author

I wasn't sure how you'd like the wording / listing of supported platforms.
Let me know if this should be different in case there are preferences otherwise.

Update the supported platforms to include RHEL8 as well. Support for this was added with theforeman#335 but this change didn't update the README as well.
README.md Outdated
@@ -19,7 +19,7 @@ This module is designed to setup a full katello server, including candlepin, pul

## Limitations

* EL7 (RHEL7 / CentOS 7)
* Supports EL7 (RHEL7 / CentOS 7) and EL8 (RHEL8 / CentOS 8)
Copy link
Member

Choose a reason for hiding this comment

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

How about we refer users to operatingsystem_support in metadata.json for the exact OS support?

Copy link
Contributor Author

@moritzdietz moritzdietz Apr 15, 2021

Choose a reason for hiding this comment

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

Pro: one less thing to update whenever the supported OSes change (but how often does that really happen)
Cons: another click and another resource for the user to check.

Having it in the README has the upside that one can see right away what's what.
IMO I'd leave it there but obviously it's up to you.

Copy link
Member

Choose a reason for hiding this comment

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

Pro: one less thing to update whenever the supported OSes change (but how often does that really happen)

This is what I'm mostly after. As you can see, it doesn't happen enough that we tend to mess it up. Especially since not all modules have it in their README. However, our CI relies on metadata.json so that's guaranteed to be correct. So it would be my preference to refer users there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just pushed another commit with an updated text.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'm not sure how well links render on the forge but I'm ok with it.

@ekohl ekohl merged commit 91307ad into theforeman:master Apr 15, 2021
@ekohl
Copy link
Member

ekohl commented Apr 15, 2021

Thanks!

@moritzdietz
Copy link
Contributor Author

Ah I would've squashed those but thanks for doing it.
Huh yes i haven't thought about the forge so valid point.
Just thought I'd somehow link it.

@moritzdietz moritzdietz deleted the patch-1 branch April 15, 2021 15:26
@evgeni
Copy link
Member

evgeni commented Apr 15, 2021

Doesn't the forge display the metadata anyways?

@moritzdietz
Copy link
Contributor Author

moritzdietz commented Apr 15, 2021

Doesn't the forge display the metadata anyways?

Yes, it renders the contents of the metadata so that it's visible there which platform to use.
But if folks browse the GitHub repository it won't be any help there. Having the readme either specify it directly or at least link to info regarding this helps all users wherever they look at the source code. But that's just IMHO.

@wbclark wbclark added the Bug label Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question: RHEL 8 support
5 participants