Skip to content

vdirsyncer: synchronize metadata as well#4167

Merged
teto merged 1 commit intonix-community:masterfrom
somasis:vdirsyncer
Jul 4, 2023
Merged

vdirsyncer: synchronize metadata as well#4167
teto merged 1 commit intonix-community:masterfrom
somasis:vdirsyncer

Conversation

@somasis
Copy link
Copy Markdown
Member

@somasis somasis commented Jun 26, 2023

Description

Without this, the service will never synchronize things like calendar/contact
display names and calendar colors.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

@ncfavier ncfavier requested a review from teto June 26, 2023 15:45
@ncfavier
Copy link
Copy Markdown
Member

Also see the discussion at #3912 (comment)

@somasis
Copy link
Copy Markdown
Member Author

somasis commented Jun 26, 2023

I think running vdirsyncer discover shouldn't be done automatically, since it's an opportunity where the user can determine if their configuration's list of collections will cause vdirsyncer to do something unintended (like creating a collection that doesn't exist on the remote).

I used to run yes y | vdirsyncer discover as part of my own vdirsyncer.service and it caused more problems than it solved, really.

Perhaps there could be a xdg.configFile."vdirsyncer/config".onChange hook to tell the user to run vdirsyncer discover to update the program's state to accompany configuration changes?

(maybe I should have commented this on that discussion, but I got the impression that it was an old pull request that was superseded by #4078)

@teto
Copy link
Copy Markdown
Collaborator

teto commented Jun 26, 2023

So I wonder why discover is necessary ? why can't it just download stuff ? or is it valuable in case you have local files already ?

In which case we could check if folder is empty and autorun discover in that case ?
Can we run discover in a way that fails only if user interaction is needed ?

When in doubt, I would choose caution and let user decide but we could make the task easier by warning him when input is needed (aka a failed discover)

@somasis
Copy link
Copy Markdown
Member Author

somasis commented Jun 26, 2023

So I wonder why discover is necessary ? why can't it just download stuff ? or is it valuable in case you have local files already ?

There's a small explanation in a tutorial in vdirsyncer's documentation:

Because collections are added rarely, and checking for this case before every synchronization isn’t worth the overhead.

https://vdirsyncer.pimutils.org/en/stable/tutorial.html?highlight=discover#id2

In which case we could check if folder is empty and autorun discover in that case ? Can we run discover in a way that fails only if user interaction is needed ?
When in doubt, I would choose caution and let user decide but we could make the task easier by warning him when input is needed (aka a failed discover)

Checking if the directory is empty seems like a good idea to me.

There's been some discussion upstream about how to automate vdirsyncer discover. pimutils/vdirsyncer#425 The old maintainer suggests doing yes | vdirsyncer discover (this is where I got that idea from).

If we want to run discover and just fail if any user input is necessary, we could check the exit status of yes n | vdirsyncer -v warning discover, since it'll exit non-zero if we respond n to a user prompt. I think this will also prevent it from doing anything funny with collections that might be unintentional. (using -v warning is just a good idea so we don't spam the system journal when there's nothing wrong).

@teto
Copy link
Copy Markdown
Collaborator

teto commented Jun 26, 2023

If we want to run discover and just fail if any user input is necessary, we could check the exit status of yes n | vdirsyncer -v warning discover

sounds good to me.

We could also make this configurable as a nix option: discover = "yes" / "ask" / "manual".

Copy link
Copy Markdown
Contributor

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

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

I personally think that this incremental improvement is worth merging as-is. We can implement "discover" in a later PR.

Comment thread modules/services/vdirsyncer.nix Outdated
@teto
Copy link
Copy Markdown
Collaborator

teto commented Jul 2, 2023

I personally think that this incremental improvement is worth merging as-is. We can implement "discover" in a later PR.

That is the spirit of calendar merge so I am fine with that. In case we merge the current, we just need to make sure the previous discussion doesn't get lost as it's valuable.

@github-actions github-actions bot added the calendar vdirsyncer, pimsync, ikhal... label Jul 3, 2023
Without this, the service will never synchronize things like calendar/contact
display names and calendar colors.
@somasis
Copy link
Copy Markdown
Member Author

somasis commented Jul 3, 2023

Sorry for the noise, should be all good this time though.

I personally think that this incremental improvement is worth merging as-is. We can implement "discover" in a later PR.

Agreed.

@teto
Copy link
Copy Markdown
Collaborator

teto commented Jul 4, 2023

tested it. Seems to work, probably not related but the first time the job failed and asked me to run vdirsyncer discover calendar_fastmail first

@teto teto merged commit d895a77 into nix-community:master Jul 4, 2023
@somasis somasis deleted the vdirsyncer branch July 7, 2023 15:58
antholeole pushed a commit to antholeole/home-manager that referenced this pull request Jul 24, 2023
SimSaladin added a commit to SimSaladin/home-manager that referenced this pull request Aug 26, 2023
* upstream/master: (352 commits)
  flake.lock: Update
  qcal: add module
  bat: generate cache file in XDG cache home
  nixos: only refer to `users.users` if needed
  home-cursor: remove IFD when linking icon directories
  flake.lock: Update
  home-cursor: improve icon compatibility
  fix(qt): allow theming for apps started by systemd (nix-community#4349)
  flake.lock: Update
  fcitx5: add `fcitx5-config-qt` to test stub
  pqiv: only run tests on Linux platforms
  pqiv: add module
  Translate using Weblate (German)
  xplr: add module
  browserpass: support librewolf
  Translate using Weblate (Romanian)
  Translate using Weblate (Portuguese (Brazil))
  hyprland: remove xwayland.hidpi (nix-community#4302)
  home-manager: handle profile list in Nix >2.17
  lf: simplify option validation (nix-community#4334)
  programs.khal: uncomment locale config (nix-community#4319)
  kanshi: support adaptive sync (nix-community#4328)
  mu: add package option (nix-community#4325)
  modules: types.string throws error now (nix-community#4324)
  taffybar: Avoid restarting too quickly (nix-community#4316)
  aerc: do not use smtp-starttls (nix-community#4272)
  zsh: Add `zsh.history.ignoreAllDups` config option (nix-community#4248)
  jujutsu: fix zsh completion (nix-community#4305)
  Translate using Weblate (Korean)
  Translate using Weblate (Romanian)
  Translate using Weblate (German)
  flake.lock: Update
  network-manager-applet: remove `--sm-disable` flag
  himalaya: update a link to ticket
  nushell: deprecation of let-env (nix-community#4292)
  man: fix caches generation in cross-compiled system (nix-community#4294)
  fish: fix session vars build in cross-compiled system (nix-community#4293)
  Update translation files
  Translate using Weblate (Chinese (Simplified))
  gh: option to enable helper for additional hosts (nix-community#4288)
  home-manager: rework news command
  home-manager: skip manual in uninstall configuration
  gnome-terminal: add assertion on profile names
  hyprland: use `toKeyValue`'s `indent` argument (nix-community#4274)
  gpg: fix typo (nix-community#4277)
  firefox: make package nullable (nix-community#4113)
  git-sync: add news entry for darwin
  tmate: don't generate empty config file (nix-community#4271)
  gh-dash: add module
  git-sync: add darwin support
  flake.lock: Update
  hyprland: prioritize variables and beziers (nix-community#4263)
  hyprland: add module
  script-directory: fix documentation link (nix-community#4258)
  Translate using Weblate (Indonesian)
  Translate using Weblate (Ukrainian)
  Translate using Weblate (Spanish)
  Translate using Weblate (Swedish)
  docs: hide `_module.*` from NixOS/nix-darwin docs
  jujutsu: update for Jujutsu 0.8.0 (nix-community#4250)
  ripgrep: don't set env. variable if no config (nix-community#4254)
  docs: update for Markdown migration
  treewide: remove now-redundant `lib.mdDoc` calls
  docs: clean up after Markdown migration
  flake: remove temporary nixpkgs pin
  treewide: convert all option docs to Markdown
  treewide: adjust some DocBook for conversion
  treewide: `mkAliasOption` -> `mkAliasOptionMD`
  treewide: `mkPackageOption` -> `mkPackageOptionMD`
  treewide: convert options with tables to Markdown
  treewide: convert options with lists to Markdown
  treewide: convert custom `enable` docs to Markdown
  treewide: convert parameterized docs to Markdown
  treewide: manually convert some docs to Markdown
  treewide: fix `mkEnableOption` arguments
  treewide: add missing option descriptions
  docs: use `nixosOptionsDoc`
  flake: temporarily pin nixpkgs version
  version: add `isReleaseBranch`
  manual: add test
  Update translation files
  flake.lock: Update
  home-manager: remove Home Manager default paths
  imapnotify: Use JSON type for extraConfig (nix-community#4238)
  Translate using Weblate (French)
  zsh: fix custom syntax highlighting styles (nix-community#4236)
  i3-sway: allow arbitrary floating modifier (nix-community#4229)
  i3-sway: multiple outputs (nix-community#4223)
  home-cursor.nix: enable gtk module when enabling gtk config generation (nix-community#4144)
  aerc: add assertion to limit per-account extraConfig to UI config (nix-community#4196)
  flake.lock: Update
  ssh-agent: add assertion and fix news entry (nix-community#4210)
  imapnotify: move test
  imapnotify: use direct nix store path for config
  flake.lock: Update
  nushell: add login.nu configuration option
  himalaya: fix notmuch backend
  swayosd: add module
  pyenv: add module
  darcs: add module
  tests: some minor cleanups
  goimapnotify: remove test dependency on notmuch
  unison: Allow using same option multiple times (nix-community#4208)
  imapnotify: Add launchd agent (nix-community#3291)
  i3: remove deprecated example in i3.config.startup (nix-community#4201)
  antidote: static file move to /tmp
  Translate using Weblate (Indonesian)
  PULL_REQUEST_TEMPLATE.md: add maintainer cc section (nix-community#4193)
  programs.khal
  vdirsyncer: synchronize metadata as well (nix-community#4167)
  i18n: Use glibcLocales from NixOS if possible (nix-community#2333) (nix-community#4177)
  xfconf: remove properties with null values (nix-community#4192)
  home-manager: Use `path:` URI type for flake default (nix-community#3646)
  ci: autolabel calendars PRs (nix-community#4165)
  flake.lock: Update (nix-community#4161)
  news: fix typo in programs.zsh.antidote entry
  ssh-agent: init module (nix-community#4178)
  chromium: fix `commandLineArgs` to use the user specified package (nix-community#4175)
  sxhkd: allow usage of derivations as keybind commands (nix-community#4169)
  README: Remove the pills (nix-community#4181)
  lsd: use -A instead of -a in aliases (nix-community#4173)
  kakoune: add defaultEditor option
  docs: update link to allowed users setting (nix-community#4176)
  zsh: allow setting custom syntax highlighting styles (nix-community#4122)
  flake: add formatter (nix-community#3620)
  broot: fix test (nix-community#4170)
  antidote: fix .dot path
  ci: build manual and push to home-manager.dev
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

calendar vdirsyncer, pimsync, ikhal...

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants