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

Fixes for ZEP source #215

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

Fixes for ZEP source #215

wants to merge 3 commits into from

Conversation

timonegk
Copy link
Collaborator

This PR introduces the following changes:

  • correctly set the calendarID for zep: previously, it was always an empty string.
  • correctly identify all-day events: zep represents them as events starting and ending at 00:00:00
  • rename absences to events: zep can have other events than absences, so events is a more fitting variable name

Correctly setting the calendarID introduces a breaking change because events synced from this source are no longer recognized in the metadata. How do you want to handle it? We could either set the calendarID to an empty string in the generateCalendarID function to not change the hash or we could say that it's okay because the change will not break much and if it breaks anything, it will only be until the end of the current sync period (i.e., for most users probably the end of the next month).

Copy link
Collaborator

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

Correctly setting the calendarID introduces a breaking change because events synced from this source are no longer recognized in the metadata. How do you want to handle it?

Hmm, it would be possible to emulate the old behavior by temporarily allowing the user to set the calendarID to an empty string. Then we could document how to migrate to the new behavior: set an empty calendarID, run with --clean, explicitly set calendarID and sync again. That would at least avoid a manual cleanup although it's also not ideal.

internal/adapter/port/interface.go Show resolved Hide resolved
@timonegk
Copy link
Collaborator Author

Hmm, it would be possible to emulate the old behavior by temporarily allowing the user to set the calendarID to an empty string. Then we could document how to migrate to the new behavior: set an empty calendarID, run with --clean, explicitly set calendarID and sync again. That would at least avoid a manual cleanup although it's also not ideal.

I think that would be quite a complicated workflow. What do you think of adding an if calenderID == "absences": calenderID = "" to the getCalendarHash function for zep and adding a comment for the reason? That would keep the hash as it was before, but would make it possible to use other zep calendars if that's ever necessary.

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