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

[Docs] Specify Apache 2.4 required in README #3976

Merged
merged 7 commits into from
Oct 18, 2018
Merged

[Docs] Specify Apache 2.4 required in README #3976

merged 7 commits into from
Oct 18, 2018

Conversation

johnsaigle
Copy link
Contributor

Brief summary of changes

Specifiy that Apache 2.4 is required.

Heroku deploy is no longer "new" so that is removed.

This addresses issue...

@johnsaigle johnsaigle changed the title [Docs] Update readme [Docs] Specify Apache 2.4 required in README Oct 1, 2018
@johnsaigle johnsaigle added Cleanup PR or issue introducing/requiring at least one clean-up operation Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) [branch] minor labels Oct 1, 2018
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

Please send this one to bugfix. 2.4 is required for 20.x, so it should be documented there too.

@johnsaigle johnsaigle changed the base branch from minor to bugfix October 1, 2018 17:33
@johnsaigle
Copy link
Contributor Author

@driusan done!

HenriRabalais
HenriRabalais previously approved these changes Oct 1, 2018
driusan
driusan previously approved these changes Oct 1, 2018
Jkat
Jkat previously approved these changes Oct 1, 2018
README.md Outdated
@@ -25,9 +25,9 @@ Please consult the [LORIS Wiki Setup Guide](https://github.com/aces/Loris/wiki/S
# Prerequisites for Installation

* LINUX (supported on Ubuntu 14+ and [CentOS 6.5](https://github.com/aces/Loris/blob/master/README.CentOS6.md))
Copy link
Contributor

Choose a reason for hiding this comment

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

might want to also specify CentOS 7 here since apache 2.4 doesn't work on < 7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, didn't know!

Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as we're on the topic, maybe we should take out the line entirely. LORIS doesn't have any Linux-only dependencies.

@johnsaigle johnsaigle dismissed stale reviews from Jkat, driusan, and HenriRabalais via 55d3c9b October 1, 2018 20:33

<b>Important:</b>
* If you are upgrading your LORIS, you'll also want to upgrade to both PHP 7.2 and MySQL 5.7.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still seems worth having.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You don't think it's redundant given that we have a bulleted list of prereqs above?

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, that makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I don't think "Important" is a very good title anymore. "Note" might be more accurate. If you don't specify --no-dev, you just get more dependencies installed.

* Apache **2.4** or higher
* MySQL 5.7
* PHP <b>7.2</b> or higher
* Package manager (for LINUX distributions)
* Composer : should be run with --no-dev option
Copy link
Collaborator

Choose a reason for hiding this comment

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

composer is still required. But maybe the line above it about needing a package manager should be removed while we're at it?

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 removed this line because it is basically a duplicate from line 36.

I'll remove the line above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would make more sense to me to keep it here and delete it below, because installing it is still a prerequisite.

README.md Outdated
* MySQL 5.7
* PHP <b>7.2</b>
* Apache **2.4** or higher
* MySQL 5.7
Copy link
Contributor

Choose a reason for hiding this comment

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

or higher?
Could we also mention MariaDB? even if we haven't tested it fully

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think @ridz1208 has tested it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which version?

Copy link
Collaborator

Choose a reason for hiding this comment

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

10.3, I think. @ridz1208 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ridz1208 The MariaDB version is the only thing preventing this from being merged

Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

I am trying the new suggest changes feature.

README.md Outdated Show resolved Hide resolved
@driusan driusan merged commit 4ccb32a into aces:bugfix Oct 18, 2018
@ridz1208 ridz1208 added this to the 20.0.2 milestone Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) Cleanup PR or issue introducing/requiring at least one clean-up operation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants