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

Automatically refresh ACL information in sharing db #422

Merged
merged 8 commits into from
Dec 8, 2021

Conversation

sampsapenna
Copy link
Member

@sampsapenna sampsapenna commented Dec 3, 2021

Description

Add automatic synchronization for ACL information in sharing database. I.e. current container access additions, removals, edits and container removals.

Previously the sharing information had a tendency towards containing stale data, since people would also access the sharing information in the CLI, remove containers, etc. leading to ACLs changing without updates.

Related issues

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update (this needs a follow up PR)

Changes Made

  • Make ACL synchronization run automatically upon login (scheduled on main App creation event)
  • Make ACL synchronization a proper background operation to prevent unresponsive UI during sync (force yielding execution in container ACL metadata fetch loop)

Testing

Mentions

@sampsapenna sampsapenna added the enhancement New feature or request label Dec 3, 2021
@sampsapenna sampsapenna self-assigned this Dec 3, 2021
Copy link
Contributor

@csc-felipe csc-felipe left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the ACLs yet, and I did not run it, as I haven't been running the whole environment yet. Thus, only looked at the code and made the review as a comment.

I would have preferred if the formatting had been a separate PR, as there's a lot of it.

Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

if my understanding is correct this #69 is not required any more.

We need to add a follow-up issue to add some integration (or cypress) tests for this. A test could look like:

  1. we log in we see some shared containers
  2. we update the db in the background
  3. we re-login the users we see the updated shared containers

I have not tested, however the code looks good and if we spot any issues I assume we will see them in practice.

also this needs a rebase

@blankdots blankdots linked an issue Dec 8, 2021 that may be closed by this pull request
@sampsapenna
Copy link
Member Author

if my understanding is correct this #69 is not required any more.

We need to add a follow-up issue to add some integration (or cypress) tests for this. A test could look like:

  1. we log in we see some shared containers
  2. we update the db in the background
  3. we re-login the users we see the updated shared containers

I have not tested, however the code looks good and if we spot any issues I assume we will see them in practice.

also this needs a rebase

Opened an issue for the integration tests in #424

#69 is not relevant to this PR, but it would go nicely together with this change. This PR was about keeping the database information as current as possible. I'd like to keep it open still.

Rebase will happen shortly 👍🏼

@sampsapenna
Copy link
Member Author

I'm not familiar with the ACLs yet, and I did not run it, as I haven't been running the whole environment yet. Thus, only looked at the code and made the review as a comment.

I would have preferred if the formatting had been a separate PR, as there's a lot of it.

Yup, the formatting was an unfortunate mishap with vscode auto-formatting that I noticed too late, thus it made its way in

@sampsapenna sampsapenna force-pushed the feature/sharing-info-autosync branch from ddd5158 to 94d71b4 Compare December 8, 2021 13:48
@sampsapenna sampsapenna merged commit 6c64bfb into devel Dec 8, 2021
@sampsapenna sampsapenna deleted the feature/sharing-info-autosync branch December 8, 2021 14:09
@sampsapenna sampsapenna mentioned this pull request Jan 12, 2022
8 tasks
@sampsapenna sampsapenna restored the feature/sharing-info-autosync branch March 16, 2022 14:58
@blankdots blankdots deleted the feature/sharing-info-autosync branch July 4, 2023 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better ACL information synchronization
3 participants