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
deconz: Add LEDVANCE / OSRAM otau firmware downloader #983
Conversation
@pvizeli, I think that PR do respect what you asked me to rectify on my previous PRs ? |
Please address the lint issue. The script have the correct permission of a+x? |
Fix lint issues: https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt... https://www.shellcheck.net/wiki/SC2003 -- expr is antiquated. Consider rewr...
Fix lint issue: https://www.shellcheck.net/wiki/SC2004 -- $/${} is unnecessary on arithmeti...
Adressing @pvizeli comment : The script have the correct permission of a+x? Add CMD to set a+x permission on ledvance-otau-dl.sh script
it's done |
deconz/Dockerfile
Outdated
COPY data/nginx.conf /etc/nginx/nginx.conf | ||
COPY data/run.sh data/discovery.sh / | ||
|
||
CMD ["/run.sh"] | ||
CMD ["/bin/chmod a+x /bin/ledvance-otau-dl.sh && /run.sh"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting permissions should be either in the Docker build or just ensure it is correctly set into the repository. Either way, not in the RUN command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sorry, i’ll look to fix that this evening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between that two methods, does there is one to be prefered ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to use git-index: git update-index --chmod=+x path-name-shell-script
So you need just copy the file on docker build like the other scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed.
Hello @olijouve, When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es). The commits that are missing a linked GitHub account are the following:
Unfortunately, we are unable to accept this pull request until this situation is corrected. Here are your options:
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community. Thanks, I look forward to checking this PR again soon! ❤️ |
Hello @olijouve, When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es). The commits that are missing a linked GitHub account are the following:
Unfortunately, we are unable to accept this pull request until this situation is corrected. Here are your options:
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community. Thanks, I look forward to checking this PR again soon! ❤️ |
remove dummy comment
Hello @olijouve, When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es). The commits that are missing a linked GitHub account are the following:
Unfortunately, we are unable to accept this pull request until this situation is corrected. Here are your options:
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community. Thanks, I look forward to checking this PR again soon! ❤️ |
remove dummy comment
Hello @olijouve, When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es). The commits that are missing a linked GitHub account are the following:
Unfortunately, we are unable to accept this pull request until this situation is corrected. Here are your options:
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community. Thanks, I look forward to checking this PR again soon! ❤️ |
@olijouve You need to fix the CLA errors as described in the bot's comment. Could you take care of that? 👍 |
I tried and wasn't able to fix it 😓 the mail adress was an @hostname.local so email verification never succeeded, also script method failed then it was too much skilled for me. |
Thanks to both of you ! |
Thank you for your PR. I was expecting to find a way to put firmware in the container. |
@oncleben31, that is out of scope for this PR, besides, it the behavior we currently use for other vendors as well. |
OK great. I didn't expect a take it into account in this PR. Just for the discussion ;-) |
…#983) * Update config.json * Create ledvance-otau-dl.sh * Update run.sh * Update Dockerfile * Update CHANGELOG.md * Update CHANGELOG.md * Update ledvance-otau-dl.sh Fix lint issues: https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt... https://www.shellcheck.net/wiki/SC2003 -- expr is antiquated. Consider rewr... * Update ledvance-otau-dl.sh Fix lint issue: https://www.shellcheck.net/wiki/SC2004 -- $/${} is unnecessary on arithmeti... * Update Dockerfile Adressing @pvizeli comment : The script have the correct permission of a+x? Add CMD to set a+x permission on ledvance-otau-dl.sh script * Update Dockerfile * Add exec permissions * revert oups bad multiple cmdlines in CMD[] * revert oups bad multiple cmdlines in CMD[] * dummy commit * Update Dockerfile remove dummy comment * Update ledvance-otau-dl.sh remove dummy comment * fix bug Co-authored-by: Pascal Vizeli <pascal.vizeli@syshack.ch>
Add LEDVANCE / OSRAM otau firmware downloader, respecting max 10 DL per minute Ratelimits
resolves #985