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 2.7.4 #130

Merged
merged 68 commits into from Apr 18, 2019

Conversation

Projects
None yet
4 participants
@yalh76
Copy link
Collaborator

commented Mar 22, 2019

Problem

  • Update to 2.7.4

Solution

  • Apply example_ynh + french and english readme
  • Use of experimental helper ynh_install_ruby
  • Add custom .env.production.sample
  • Fix remove / backup / restore / upgrade scripts
  • Implement change_url

PR Status

  • Code finished.
  • Tested with Package_check.
  • Fix or enhancement tested.
  • Upgrade from last version tested.
  • Can be reviewed and tested.

Validation


  • Code review
  • Approval (LGTM)
  • CI succeeded :
    Build Status

Revision: 0494ace
Capture

@anmol26s

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2019

So, what should be done for the change_url? My vote is to remove it.

@yalh76

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 11, 2019

So, what should be done for the change_url? My vote is to remove it.

I made a Poll in #133, it request Polls to be installed in mastodon_ynh ^^

@anmol26s

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2019

@yalh76
Why you have removed this PR? #129

@anmol26s

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2019

This should be removed or changed to 2.7.4: 8bcf441#diff-04c6e90faac2675aa89e2176d2eec7d8R11

This should be removed or changed from testing to working:
8bcf441#diff-04c6e90faac2675aa89e2176d2eec7d8R12

This should be removed or changed to the version listed in manifest:
8bcf441#diff-04c6e90faac2675aa89e2176d2eec7d8R15

For me all these lines from 11 to 16 should be removed.

@yalh76

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 11, 2019

@yalh76
Why you have removed this PR? #129

I didn't removed PR #129
I forked the project before you merged it:
Sans titre

@yalh76

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 11, 2019

This should be removed or changed to 2.7.4: 8bcf441#diff-04c6e90faac2675aa89e2176d2eec7d8R11

This should be removed or changed from testing to working:
8bcf441#diff-04c6e90faac2675aa89e2176d2eec7d8R12

This should be removed or changed to the version listed in manifest:
8bcf441#diff-04c6e90faac2675aa89e2176d2eec7d8R15

For me all these lines from 11 to 16 should be removed.

All badges removed.
I did not dare to delete them thinking that it's important for the mastodon_ynh's maintainer

@anmol26s

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2019

I forked the project before you merged it

Can you fetch this PR in your PR?

Merge pull request #3 from alexAubin/patch-1
Add a check that there's a reasonable quantity of RAM+swap available
@yalh76

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 11, 2019

I forked the project before you merged it

Can you fetch this PR in your PR?

Fetched, waiting package_check results

@yalh76

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 11, 2019

I forked the project before you merged it

Can you fetch this PR in your PR?

With PR #129 package check failed... the VM doesn't have enough memory

yalh76 added some commits Apr 12, 2019

Fix postgresql database removal error
in new ynh_psql_remove_db db_name and db_user have been inverted in legacy mode
Revert "Merge pull request #3 from alexAubin/patch-1"
This reverts commit 90a0331, reversing
changes made to df2e2fb.
@anmol26s

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2019

With PR #129 package check failed... the VM doesn't have enough memory

If @alexAubin is ok to not to include this PR for now, then we can merge this.

@alexAubin

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Yes 😉 Or maybe @maniackcrudelis knows a trick to detect the script is being run on package check or the CI

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

Indeed I do...

But the problem here isn't about the CI or package_check. The problem is about the amount of ram available.
An experimental helper is available to add a swap file for those situations.

@yalh76

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2019

With PR #129 package check failed... the VM doesn't have enough memory

If @alexAubin is ok to not to include this PR for now, then we can merge this.

I made an ugly workaround to let the package_check works without PR #129

@alexAubin

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

You can add swap, but I dunno if that's the right solution. Consider a user running an Olimex board or Raspberry Pi 2 or idk ...

  • either the user knows what he's doing and agrees to add swap (despite the fact that this can ruin the SDcard if swapiness ain't configured properly)
  • either the user doesnt know what he's doing and you simply want to abort the install before it crashes
@anmol26s

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2019

I feel that feature of adding swap memory should be done through command-line rather then the package helper.

I made an ugly workaround to let the package_check works without PR #129

Lets see if package check is ok with it.

@yalh76

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2019

Lets see if package check is ok with it.

For now https://ci-apps-dev.yunohost.org/jenkins/ is down but I update the result from my own CI.

@anmol26s

This comment has been minimized.

@yalh76

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 13, 2019

I've seen that.

I made the same build on my own CI and updated the screenshot: The build is OK.
http://www.ipv6proxy.net/go.php?u=https%3A%2F%2Fvm00.yalh.net%2Fjenkins%2Fview%2Fmastodon%2Fjob%2Fmastodon_yalh76_develop%2F&b=1

Maybe something specific on ci-apps-dev ?

@anmol26s

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

@maniackcrudelis will give more inside to it. Sometimes my apps fails completely then on next check they rise up.

@anmol26s

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

Should we merge it with CI status failing? The app can't be improved till its tested by users and issues are reported.

@yalh76

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 18, 2019

I think that we can merge it.
The build is ok in my CI. And the error in the dev-ci is related to nodejs installation using official helper...

@anmol26s

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

Ok, merging this. If there is a problem it can be fixed in develop before merging to master. For me code looks good.

@anmol26s anmol26s merged commit 11f2ee8 into YunoHost-Apps:develop Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.