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

ynh_permission_* helper should not be depreciated #128

Open
Josue-T opened this issue Feb 14, 2024 · 5 comments
Open

ynh_permission_* helper should not be depreciated #128

Josue-T opened this issue Feb 14, 2024 · 5 comments

Comments

@Josue-T
Copy link
Contributor

Josue-T commented Feb 14, 2024

On some apps like synapse we must enable conditionally some permission. The declarative way in the manifest.toml is great but is also limited. In some case we need more flexibility it's why we still need theses helper to manage theses cases.

@tituspijean
Copy link
Contributor

tituspijean commented Feb 14, 2024

Reference: https://github.com/YunoHost-Apps/synapse_ynh/blob/b0850e5e517fbc1f300f974a62709977da7684f5/scripts/install#L425-L429

Though, would we actually need that condition if we were using packaging v2, with a domain question in the manifest instead of a string?

@alexAubin
Copy link
Member

On one hand, yes, but on the other hand, the point of declarativeness is that you're supposed to know what state to configure just by reading a static file and not having to run any epic install/update/whatever script. If we allow .1% of apps to do "stuff only in scripts" then this means we can't develop any stuff like yunohost app regen-conf that would regen bunch of configurations from the core without calling any app-specific script ...

So imho this just means that we have a shortcoming in the current packaging format, and we should keep track of it. I guess ultimately this means we should have so if = statements in the declarative format with bash snippets (similar to packages_from_raw_bash for the apt resource) but the exact design should also take into account what are the most common use cases to cover

@Josue-T
Copy link
Contributor Author

Josue-T commented Feb 14, 2024

Well,

For synapse it's quite specific because we have the domain which is where we provide the service. And on other side we have the server_name which is independent (which could be same) which could be anything the user want. The only constraint is that the user have access to manage it.

And for this part it could be a domain managed by yunohost but it could also be anything else that the user manage somewhere else. In the case of yunohost manage it, we try to configure the .well-known to make it working without any configuration. But if the domain is not managed by yunohost we just don't want to crash because of the domain for the permission don't exist as it's not mandatory. So it's why there are a if.

@Josue-T
Copy link
Contributor Author

Josue-T commented Feb 14, 2024

But yes I agree with the idea of a declarative way of if = but currently it's not implemented so the current workaround is to use something like this which is not really clean.

And as same as for this (YunoHost/issues#2245) we depreciate the db helper but we still need it for some apps like seafile.

@Tagadda
Copy link
Contributor

Tagadda commented Feb 14, 2024

To ensure that only the user selected as admin can access the app I use ynh_permission_update for code-server_ynh in the install and upgrade scripts. https://github.com/search?q=repo%3AYunoHost-Apps%2Fcode-server_ynh%20permission&type=code
I think this behavior is needed for some other apps like Kresus.

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

No branches or pull requests

4 participants