xddq / libre-weather

Libre Weather is an open source, privacy respecting, self hostable progressive web app. Can be downloaded for iOS and Android.
GNU General Public License v3.0
23 stars 1 forks source link

Suggestions #1

Closed spinside closed 2 years ago

spinside commented 2 years ago

Hi,

Thanks for creating this awesome tool. just a couple of questions/suggestions:

xddq commented 2 years ago

Hi spinside,

thanks for the kind words! I am glad you can make use of it. To answer your points:

  1. This point depends. I am not sure about the law here. If it is legal to store the last search location inside the cookie (without adapting the cookie banner) this is a relatively quick fix. I think (I am not sure, and therefore did not implement it yet) then we have to mention something like "personalization cookies" in the banner and perhaps also make them confirm and denyable. --> more work.

If you find some evidence for either side it would be a great help. My answer will be depending on the result.

  1. That is a good idea as well. I might tackle this one on my spare this this week. I suppose this would require the dockefile to only build the app. Then create a setup or entrypoint which will add all ENV vars into .env and then start the webserver. With registry, you are talking about the docker registry, correct?

If you want to help out on any of this, feel free. If you have questions, ask ahead.

spinside commented 2 years ago

Hi,

  1. I think it depends a bit on the region. To my knowledge the European GDPR is the most strict about it. gdpr.eu Mentions on their web page the region for weather information as a functional cookie.

Preferences cookies — Also known as “functionality cookies,” these cookies allow a website to remember choices you have made in the past, like what language you prefer, what region you would like weather reports for, or what your user name and password are so you can automatically log in.

In my opinion saving your location can be viewed as essential functionality. On the other hand, mentioning the cookie is used to set your location preference could not hurt either.

  1. Yes correct, that was exactly what I was thinking about.

Thanks for taking the time to respond to me :)

spinside commented 2 years ago

Extension to the second point, I had some time to check the code and if I'm correct you are using: dotenv

This package already support environment variables:

Additionally, you can use environment variables to set configuration options. Command line arguments will precede these. They should be prefixed with: DOTENV_CONFIG_<OPTION>=value This is onle possible if you use preload.

For the creation of Docker images something like this could help us:

name: build-publish-docker-on-release

on:
  release:
    types: [published]

jobs:
  login:
    runs-on: ubuntu-latest
    steps: 
  - uses: actions/checkout@v2
    name: Check out code

  - uses: mr-smithers-excellent/docker-build-push@v5
    name: Build & push Docker image
    with:
      image: xddq/libre-weather
      tags: ${{ github.event.release.name }}, latest
      registry: registry.hub.docker.com
      dockerfile: dockerfile
      username: ${{ secrets.DOCKER_USERNAME }}
      password: ${{ secrets.DOCKER_TOKEN }}

This can be added in .github/workflows/ It makes use of Github actions and you only need to set your Docker username and token in a Github secret.

It will create an Docker image on every release.

let me know what you think.

xddq commented 2 years ago
  1. Nice! We can completely drop the cookie consent component then. I also read a bit. It looks like there shall still exist a small cookie policy page explaining what the cookies are doing.

  2. Ahh, good catch with the dotenv module. This will be the way we are going to build it then, I suppose. The workflow suggestion is also good, but I prefer to use .gitlab-ci.yml for this, as this repo is somewhat of a mirror of a self managed gitlab instance and I don't really want to depend or get into github to much.

If you are experienced with docker and gitlab ci you are more than welcome to create a PR for the auto deployment (or start working on it and I will finish it) but my time is currently pretty limited.

I will now take the rest of my night to try to fix the current bug with the show/hide weather chart buttons. Then check how far I will get with the settings! And if I am lucky, perhaps add the cookie policy page.

xddq commented 2 years ago

@spinside v1.4.0 is up : ]

I probably can't/won't work on the second point for the next 1-2 weeks. If you can/want to work on it, it is appreciated. Feel free to send a PR.

spinside commented 2 years ago

@xddq thank you this is great!

For the Gitlab-ci I came up with the following:

image: docker:20.10.12

services:
  - docker:20.10.12-dind

variables:
  DOCKER_HOST: tcp://docker:2376
  DOCKER_TLS_CERTDIR: "/certs"

before_script:
  - docker login -u $CI_REGISTRY_USER -p $CI_REGISTRY_PASSWORD $CI_REGISTRY

build_image:
  stage: build
  only:
    - tags

  script:
    - docker build -t my-docker-image .
    - docker push xddq/libre-weather:${CI_COMMIT_REF_NAME}
xddq commented 2 years ago

Ugh, reminder for myself:

@spinside anyway, it's done : ) I took the base of your gitlab-ci file, but had to adapt more than initially expected. But you can check it out yourself here

Would be great of you could reassure me that I didn't miss some security stuff or that you got all that you need/wanted by now.

I will (now for real) not do any more commits on this for the next 1-2 weeks since I want to get my other projects going. I will still answer questions, review and merge pull requests.

Make sure to close the issue if it has been resolved :]

spinside commented 2 years ago

Hi @xddq,

Thank you for taking the time. Sorry I missed the part where you also wanted to autodeploy to your own server. If I knew I would have started something for that as well.

It looks very good, only thing I can spot is the use of echo for filling the env vars. Won't this add duplicates after multiple deploys? maybe either delete the .env file first or use sed to update. E.g.: sed "s/API_KEY=.*/API_KEY==${API_KEY}/g"

If not just ignore above ;)

Very happy with the changes and thank you for developing this awesome libre application!