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

read customized values if they exists to make hook works. #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

adglkh
Copy link

@adglkh adglkh commented Feb 27, 2017

Currently, it doesn't read settings from $SNAP_COMMON/wekan_settings.sh
which results in regarding customized settings doesn't take effect.
To verify it
sudo snap set wekan-ondra port=9876
sudo snap disable wekan-ondra
sudo snap enable wekan-ondra

Then navigate to localhost:9876 in your browser to check if you can open wekan website from you local.

Currently, it doesn't read settings from $SNAP_COMMON/wekan_settings.sh
which results in regarding customized settings doesn't take effect.
e.g. port
Copy link
Owner

@kubiko kubiko left a comment

Choose a reason for hiding this comment

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

source $SNAP_COMMON/wekan_settings.sh should not be needed, as this is called from $SNAP/bin/config, check line 8. But export there is indeed missing.

Copy link
Owner

@kubiko kubiko left a comment

Choose a reason for hiding this comment

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

Actually export there is not needed either, as export is already done inside $SNAP_COMMON/wekan_settings.sh
But I just noticed that we export there also keys which were not set, and that is bug

be lower case instead of upper case. And revert the customized values
reading from wekan_settings.sh file.
@adglkh
Copy link
Author

adglkh commented Feb 28, 2017

If we export keys which were not set, they will be overridden with default key value. See $SNAP/wekan-read-settings file So that's not a bug.
But I suppose that end-user wrongly sets the configure hooks after reading the description by using the upper case key, there will be no chance for them to get it work.
I fixed them in description section in yaml file. Please have a check

@adglkh
Copy link
Author

adglkh commented Feb 28, 2017

Basically, if the keys which were not set will be overridden by using the default value.
So it's not a bug.
It turns out that if normal use sets configure hooks with upper case key which is actually should be lower case key. That's why it doesn't take effect.
Please take a look description in yaml file.

@kubiko
Copy link
Owner

kubiko commented Mar 6, 2017

snapcraft yaml is out of sync. I will remove settings description from there completely, as now you can get all info by calling wekan.help. That is cleaner implementation as that reads from src/config key mapping rules. Like this we have central place where all key mapping and all other places are reading it from there.
Thanks for spotting this!
I also fixed now problem with handling empty keys, just did not have time to fix that before MWC :(

@adglkh
Copy link
Author

adglkh commented Mar 7, 2017

I like the idea of calling wekan.help to list all available key names from the command line. Indeed, It's cleaner than the hooks-fix branch implementation that just lists all of them in yaml file.
Could you please bump up the version and release the latest version into store?
Thanks.

@kubiko
Copy link
Owner

kubiko commented Mar 7, 2017

I have it set on auto build, so all changes are already in the store. But I just noticed they have mad 0.12 release. So now building it

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.

2 participants