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

Allow overriding is_webapp parameter in tests.toml #149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jakski
Copy link

@Jakski Jakski commented Jan 22, 2024

package_check tries to grep for ^ynh_add_nginx_config in order to decide whether tests with URL should be launched at all. I put ynh_add_nginx_config invocation in scripts/_common.sh in order to reuse some code between install and upgrade as a functions. With this approach package_check attempts to install my application only with nourl scenario. That's not correct. The following change makes it possible to override is_webapp parameter during tests by setting webapp in tests.toml.

Note that invoking ynh_add_nginx_config conditionally like:

if [ "$something" = 1 ]; then
    ynh_add_nginx_config
fi

would also make package_check treat application as a non-webapp, even though that's not true.

@Salamandar
Copy link
Contributor

IMHO this configuration would be better in the tests.toml :)

@Jakski
Copy link
Author

Jakski commented Jan 22, 2024

I agree, but for now I've left auto-detection using grep in order to not break compatibility with already existing applications.

@Salamandar
Copy link
Contributor

Ah oops, i misread, now I see you already did that :)

@alexAubin
Copy link
Member

We could also just grep _common.sh in addition to install

TBH we tend to discourage to "factorize" this kind of stuff like the add_nginx call etc ... Yes it's not pretty but anyway current packaging is pretty much trash (though less worse than packaging v1) until we implement packaging v3 ... Until then it's better to stick to common practice to facilitate community maintenance of all apps even though it's tempting to factorize stuff...

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.

3 participants