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

fix: reset watch of authfile after delete event #523

Merged

Conversation

giuseppelillo
Copy link
Contributor

@giuseppelillo giuseppelillo commented Jan 25, 2023

About this change - What it does

awatch disables file watching if a delete event for that file is issued. This may result in the impossibility to read new users and permissions data if the authfile is replaced.

Why this way

Resetting file watch after delete event allows notifications related to the new file to be received.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jan 25, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3e2f0a8
Status: ✅  Deploy successful!
Preview URL: https://6329e5e1.karapace.pages.dev
Branch Preview URL: https://giuseppelillo-rewatch-auth-f.karapace.pages.dev

View logs

@giuseppelillo giuseppelillo force-pushed the giuseppelillo/rewatch-auth-file-after-delete-event branch from 304ddea to 3e2f0a8 Compare January 26, 2023 13:12
@giuseppelillo giuseppelillo changed the title Reset watch of auth file after delete event Reset watch of authfile after delete event Jan 26, 2023
@giuseppelillo giuseppelillo changed the title Reset watch of authfile after delete event fix: reset watch of authfile after delete event Jan 26, 2023
@giuseppelillo giuseppelillo marked this pull request as ready for review January 26, 2023 13:36
@giuseppelillo giuseppelillo requested review from a team as code owners January 26, 2023 13:36
Copy link
Contributor

@jjaakola-aiven jjaakola-aiven left a comment

Choose a reason for hiding this comment

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

LGTM

@jjaakola-aiven jjaakola-aiven merged commit f0823df into main Jan 26, 2023
@jjaakola-aiven jjaakola-aiven deleted the giuseppelillo/rewatch-auth-file-after-delete-event branch January 26, 2023 14:10
Comment on lines +94 to +96
changes_by_filename = defaultdict(set)
for change, filename in changes:
changes_by_filename[filename].add(change)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks a bit odd to me to be dealing with multiple filenames here, when we're only ever interested in a single one.

Perhaps instead we could just flatten the data into a set for only the interesting file?

changes = {
    change
    for  change, filename in changes
    if self._auth_filename in filename
}
if not changes:
    continue

I think we'd then be able to skip the for loop and conditional guarding self._load_authfile() too?

This would also allow simplifying the raise StopIteration into just break, I think?

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.

3 participants