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

upgrade to 1.20, fix nginx issue, open jsonrpc for kandroid #55

Merged
merged 3 commits into from Mar 15, 2018
Merged

upgrade to 1.20, fix nginx issue, open jsonrpc for kandroid #55

merged 3 commits into from Mar 15, 2018

Conversation

Jibec
Copy link
Contributor

@Jibec Jibec commented Jan 18, 2018

only tested with package_check, please have a look with real installs, i'll do the same on my server soon

Problem

  • Description of why you made this PR

Solution

  • And how you fix that

PR Status

Obviously, you should really check these affirmations
Work finished. Package_check, basic tests and upgrade from last version OK.
Could be reviewed and tested.

Validation


Minor decision

  • Upgrade previous version : Maniack C
  • Code review : Maniack C
  • Approval (LGTM) : JimboJoe
  • Approval (LGTM) : Maniack C
  • CI succeeded : Build Status
    When the PR is mark as ready to merge, you have to wait for 3 days before really merge it.

@Jibec
Copy link
Contributor Author

Jibec commented Jan 18, 2018

tested with package_check, VM upgrade, and personal server upgrade works fine.
I have no plugins, only a few projects with a few hundreds tasks

@JimboJoe JimboJoe requested a review from a team January 19, 2018 21:27
@JimboJoe
Copy link
Contributor

LGTM, code review and upgrade from previous version OK... except for the Kandroid part: I didn't manage to login from Kandroid...
I guess you need to use skipped_uris instead of unprotected_uris as in ttrss.

@Jibec
Copy link
Contributor Author

Jibec commented Jan 29, 2018

can we go forward? The connection bug is not related to Kanboard: andresth/Kandroid#38

@JimboJoe
Copy link
Contributor

Didn't we need to activate LDAP authentication...?

@maniackcrudelis
Copy link
Contributor

Code review OK and LGTM

@maniackcrudelis
Copy link
Contributor

Will be merged in 3 days.

Copy link
Member

@frju365 frju365 left a comment

Choose a reason for hiding this comment

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

I wanted just to know, why you removed the maintainer mbugeia ? (I think even if he is not here, we should keep his name, at least to be ok with the copyright allowed by the license). Btw, you can add your name near the other one :) 2 maintainer is ok too. At least it's my point of view.

@maniackcrudelis
Copy link
Contributor

By the way, it could be really interesting to have a way to keep traces of the previous maintainers.
I can understand that now jibec is becoming the main maintainer, but it would be really nice to keep mbugeia somewhere too.

@Jibec
Copy link
Contributor Author

Jibec commented Mar 12, 2018

if you tell me how to do it, I'll be pleased to add previous maintainer name as an INFO in the manifest.json, but I doubt this file aim to keep historic data. I would rather keep it somewhere else (like Readme, but we don't need structured data for this).

@maniackcrudelis
Copy link
Contributor

I opened a issue about that, https://dev.yunohost.org/issues/1095
Let's talk about how handle this over there.

For the moment, an upgrade of kanboard could be interresting and should not be stopped by that.
This field is about the current maintainer, and we have to keep it clear to allow users to ask to the real (and current) maintainer of an app.

As this PR has been accepted, I merge it.

@maniackcrudelis maniackcrudelis merged commit fc6bcc6 into YunoHost-Apps:testing Mar 15, 2018
@maniackcrudelis
Copy link
Contributor

if you tell me how to do it, I'll be pleased to add previous maintainer name as an INFO in the manifest.json, but I doubt this file aim to keep historic data. I would rather keep it somewhere else (like Readme, but we don't need structured data for this).

Following our meeting this evening, the idea is to try a new entry in the manifest called "old_maintainers" or "previous_maintainers", which should not causes any trouble to the manifest.
I like the idea to keep this information just next to the current maintainer.

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

Successfully merging this pull request may close these issues.

None yet

4 participants