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

Manifestv2 #84

Closed
wants to merge 8 commits into from
Closed

Manifestv2 #84

wants to merge 8 commits into from

Conversation

Salamandar
Copy link
Member

@Salamandar Salamandar commented Sep 9, 2023

Problem

Manifestv1 !

Solution

Manifestv2 !

PR Status

  • Code finished and ready to be reviewed/tested
  • The fix/enhancement were manually tested (if applicable)

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

Fingers crossed!
Test Badge

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

Meow 🐈
Test Badge

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

Meow 🐈
Test Badge

@ericgaspar
Copy link
Member

@Salamandar I made a branch with my version 2. I gave up at one point, so feel free to poke around (I merged your huge source list)

@Salamandar
Copy link
Member Author

@Salamandar I made a branch with my version 2. I gave up at one point, so feel free to poke around (I merged your huge source list)

Oh alright ! got it :)

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

Alrighty!
Test Badge

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

May the CI gods be with you!
Test Badge

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

Fingers crossed!
Test Badge

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

✌️
Test Badge

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

May the CI gods be with you!
Test Badge

@Salamandar
Copy link
Member Author

!testme

1 similar comment
@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

Alrighty!
Test Badge

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

😜
Test Badge

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

✌️
Test Badge

@Salamandar Salamandar changed the title Draft: Manifestv2 Manifestv2 Sep 12, 2023
@Salamandar
Copy link
Member Author

I still have to ask gitea guys if we really need every minor version of gitea for database upgrades

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

Fingers crossed!
Test Badge

@Salamandar
Copy link
Member Author

!testme

conf/app.ini Outdated Show resolved Hide resolved
@ericgaspar
Copy link
Member

email config: we need to talk with the local email server, via 127.0.0.1, not DOMAIN.

Well, I've tested all mail stack (sending AND receiving email) and it work, so please leave the config as it is.

fix manifest : we don’t need resources.install_dir.dir as it’s the default value, we already discussed that with @ericgaspar

By default it's stored to /var/www/$app but for me it's not a good choice to store binary file in this place, it was discussed many time, the last one was here: YunoHost-Apps/seafile_ynh#109 (comment)

If I recall correctly, the plan for future packaging is to have a specific path for apps. having /var/www/$app path as standard will facilitate automagic transition to this new path. (Same logic as transition from /opt/yunohost/$app to /home/yunohost.app/$apps.

@Josue-T
Copy link
Contributor

Josue-T commented Nov 29, 2023

Fix and understand why admin don't work any more on LDAP

What issue did you find with admin ? It works on my end.

On my side no user are set as admin on new install. It's a know issue here : #76

Add group support on LDAP

It was not supported before right ? If so, i’d prefer to handle it in another merge request :)

No only permissions are supported now. Link with user-group are not managed but I need to investigate what exactly is the feature of this.

Test deeply upgrade from last version (package v1)

It should work fine ("should"…). It worked on my server. The CI does not show particular issues, and once it’s merged on testing, we can ask on the forum for voluntaries to test it.

Well before merging to testing we MUST test this correctly. I already heard (like this) that it's better to merge to testing and after more people test but the issue with this strategy is that after we merge to master and there are many regression that nobody fix...

So no we must take the time to test correctly that everything work before merging this.

@Josue-T
Copy link
Contributor

Josue-T commented Nov 29, 2023

email config: we need to talk with the local email server, via 127.0.0.1, not DOMAIN.

Well, I've tested all mail stack (sending AND receiving email) and it work, so please leave the config as it is.

fix manifest : we don’t need resources.install_dir.dir as it’s the default value, we already discussed that with @ericgaspar

By default it's stored to /var/www/$app but for me it's not a good choice to store binary file in this place, it was discussed many time, the last one was here: YunoHost-Apps/seafile_ynh#109 (comment)

If I recall correctly, the plan for future packaging is to have a specific path for apps. having /var/www/$app path as standard will facilitate automagic transition to this new path. (Same logic as transition from /opt/yunohost/$app to /home/yunohost.app/$apps.

Well /opt/yunohost/$app was allways used for binary files for python, c, go, etc application while /home/yunohost.app/$apps was always used for application data and for now as I now it will stay here.

@ericgaspar
Copy link
Member

On my side no user are set as admin on new install. It's a know issue here : #76

This problem is not present in Forgejo... Maybe you could look into Forgejo package as it is the same rebranded app as Gitea

@Josue-T
Copy link
Contributor

Josue-T commented Nov 30, 2023

Hello,

Well, now I need to push some fix but I don't have any more access right. @Salamandar can you fix it ? Or does I need to create new PR with other branch ?

@Salamandar
Copy link
Member Author

Hello,

Well, now I need to push some fix but I don't have any more access right. @Salamandar can you fix it ? Or does I need to create new PR with other branch ?

Could you instead please make change requests on github ? That’ll help review and discussion :)

@Salamandar
Copy link
Member Author

@Josue-T Why did you remove the [install.admin] question ? That’s probably why you don’t have admin users…

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

Meow 🐈
Test Badge

@Salamandar
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

🎠
Test Badge

Salamandar and others added 7 commits December 2, 2023 18:08
* Move app install dir from /opt to /var/www
* Revamp the way binaries are downloaded thanks to manifestv2 (no need to check for arch, etc)
* Remove custom helpers (exec_as, ynh_handle_app_migration)
* Rename LFS_KEY as LFS_JWT_SECRET and KEY as SECRET_KEY as named in app.ini
* Add JWT_SECRET for oauth and INTERNAL_TOKEN in app.ini
* update upstream sample app.ini URL
* Disable actions for now.
* Automatically add ssh permissions to the system user
* Remove support for upgrade before 1.6.4. Edit test_upgrade_from accordingly.
* database PATH
* JWT_SECRET

cleanup app.ini
@Josue-T
Copy link
Contributor

Josue-T commented Dec 6, 2023

On my side no user are set as admin on new install. It's a know issue here : #76

This problem is not present in Forgejo... Maybe you could look into Forgejo package as it is the same rebranded app as Gitea

Forgejo as the same issue cf: https://github.com/YunoHost-Apps/forgejo_ynh/blob/master/manifest.toml#L42

And it look like an upstream issue: go-gitea/gitea#25985

@Salamandar
Copy link
Member Author

On my side no user are set as admin on new install. It's a know issue here : #76

This problem is not present in Forgejo... Maybe you could look into Forgejo package as it is the same rebranded app as Gitea

Forgejo as the same issue cf: https://github.com/YunoHost-Apps/forgejo_ynh/blob/master/manifest.toml#L42

And it look like an upstream issue: go-gitea/gitea#25985

Indeed, and I couldn’t make email reception work for now on gitea.

Would it be possible to merge this PR on testing first ? I have some other changes I need to merge (gitea logs speak about deprecated config keys, there’s also the issue of the auto-edited config file), and it would be nice to at least have this huge change merged :D

- Restart Gitea service:

```bash
systemctl stop __APP__.service
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
systemctl stop __APP__.service
systemctl restart __APP__.service


If you want to use the Git command (like `git clone`, `git pull`, `git push`), you need to set this app as **public**.
Due of the backup core only feature the data directory in `/home/yunohost.app/__APP__` **is not removed**. It must be manually deleted to purge user data from the app.
Copy link
Member

Choose a reason for hiding this comment

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

Why not, but that's the same stuff for every app with a data dir ...

Anyway, nowadays yunohost app remove has a --purge option, and i think with packaging v2 there may be a modal asking admins wether or not to purge data ?

ynh_systemd_action --service_name="$app" --action="start" --log_path="/var/log/$app/gitea.log" --line_match="Starting new Web server: tcp:127.0.0.1:"
# FIXME: Leave the time to update the database schema
sleep 5
systemctl stop "$app"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
systemctl stop "$app"
ynh_systemd_action --service_name="$app" --action="stop"

manifest.toml Show resolved Hide resolved
Comment on lines +13 to +15
# previous_maintainers = [
# "rafi59"
# ]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# previous_maintainers = [
# "rafi59"
# ]

@alexAubin
Copy link
Member

Would it be possible to merge this PR on testing first ?

+1, i don't know what's holding this PR right now, i made a few comments but these really are minor/trivialish stuff

@alexAubin alexAubin dismissed stale reviews from Josue-T and ericgaspar January 22, 2024 18:31

Fixed

@ericgaspar
Copy link
Member

!testme

@yunohost-bot
Copy link
Contributor

✌️
Test Badge

@Josue-T
Copy link
Contributor

Josue-T commented Feb 4, 2024

Note I will push some fix this week 😉

@Josue-T Josue-T closed this Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants