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

Updates #9

Merged
merged 11 commits into from
Jul 11, 2017
Merged

Updates #9

merged 11 commits into from
Jul 11, 2017

Conversation

M5oul
Copy link
Member

@M5oul M5oul commented May 23, 2017

  • Manually force level 4.
  • Linter is 🆗 which validate level 5
  • [fix] Remove double firewall allow.
  • [fix] Modify configuration file, and then move it.
  • set myself as maintainer.

The app should go level 6 and could pretend official integration.

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.

It's good. But it's better to add an issue for Level 4=1 + add the URL of the issue in the 'check_process' file.
For the maintainer, perhaps it's better to have the name of the original maintainer of the package somewhere in the manifest or at least in comment (#).

@M5oul
Copy link
Member Author

M5oul commented May 23, 2017

It's good. But it's better to add an issue for Level 4=1 + add the URL of the issue in the 'check_process' file.

Ok, I will do that.

For the maintainer, perhaps it's better to have the name of the original maintainer of the package somewhere in the manifest or at least in comment (#).

For the original contributor, his contact is on git commits. For me it's enough.
We could create a file containing Contributors. I won't do it as git commits say enough about work history of the repository.

@maniackcrudelis
Copy link
Contributor

maniackcrudelis commented Jun 2, 2017

Hum, the level 6 is reached only because yunohost return 0 even if the backup and restore are clearly failed !
And with a blank upgrade script, I should say that this app is only reaching the level 2...

Also, for official integration, some things are missing.

  • Use more helpers. YEP 2.12
  • Use a real ;) upgrade script.
  • And also have backup and restore script for the level 6.
  • The test with the port obviously fail, because your script has to fail if the port is not available

If you need some help, don't hesitate :)

@M5oul
Copy link
Member Author

M5oul commented Jun 6, 2017

Any review on this one?
@frju365 ok for you now?

@JimboJoe
Copy link

JimboJoe commented Jun 9, 2017

After reviewing it, I think @maniackcrudelis said it all. Moreover as a newer official app, it would be much better (expected?) to comply with the latest template (pending PRs on example_ynh).
I understand these reviews are a bit harsh to read so feel free to raise your voice if needed :-)

@M5oul M5oul dismissed frju365’s stale review July 11, 2017 12:45

No update since a while.

@M5oul
Copy link
Member Author

M5oul commented Jul 11, 2017

Any approval for this MR?
It only updates the package.
For official integration, we will see later. Let's use #10 for that.

@maniackcrudelis
Copy link
Contributor

I just read the code, but I have already do a check earlier.
So, ok for me for an upgrade.

@M5oul M5oul merged commit c3f4863 into master Jul 11, 2017
@M5oul M5oul deleted the updates branch July 11, 2017 19:40
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.

None yet

4 participants