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.

Sign up for GitHub

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

Jump to bottom

[bugfix] Don't check for autosync on manual triggered sync (#3026) #3029

Merged
merged 3 commits into from
Feb 27, 2025
Merged

[bugfix] Don't check for autosync on manual triggered sync (#3026) #3029

merged 3 commits into from
Feb 27, 2025

Conversation

TM2500
Copy link
Contributor

@TM2500 TM2500 commented Jan 6, 2025

  • [bugfix] Don't check for autosync on manual triggered sync

Fixes #3026

@TM2500 TM2500 mentioned this pull request Jan 6, 2025
Closed
@TM2500
Copy link
Contributor Author

TM2500 commented Jan 6, 2025

Additional Tests are needed to verify this bugfix.
As soon as they are created, the draft state is removed from the PR.

@TM2500 TM2500 force-pushed the 3026-autosync-disables-sync branch from 27bce73 to e876920 Compare January 6, 2025 14:13
AnomalRoil
AnomalRoil reviewed Jan 14, 2025
View reviewed changes
AnomalRoil
AnomalRoil reviewed Jan 14, 2025
View reviewed changes
@TM2500
Copy link
Contributor Author

TM2500 commented Jan 31, 2025

For tests I would have to write new tests utils, that support fake-remote git repos as stores. This may take me a while to figure out how to include it in the available tooling.
The bug is fixed with the last two commits.

@dominikschulz
Copy link
Member

@TM2500 Thanks for the PR. I won't press for tests if they don't provide more value than maintanence overhead. Feel free to mark this ready for review as is or whenever you think it's ready. Then I'll give it a good look and let you know if I think anything is amiss.

@TM2500
Copy link
Contributor Author

TM2500 commented Feb 16, 2025

@TM2500 Thanks for the PR. I won't press for tests if they don't provide more value than maintanence overhead. Feel free to mark this ready for review as is or whenever you think it's ready. Then I'll give it a good look and let you know if I think anything is amiss.

Yes, I think tests are an order of magnitude more work, as we would have to find a way to generate dummy-remote stores in our test suite.

I will mark this for review and the PR transitions from draft to a regular PR.

Thank you.

@TM2500
[bugfix] Don't check for autosync on manual triggered sync (gopasspw#…
850b0f3 
…3026)

* [bugfix] Don't check for autosync on manual triggered sync

Fixes gopasspw#3026

Signed-off-by: Ing. Thomas Mantl <[email protected]>
@TM2500
use correct context
69c3e75 
Fixes gopasspw#3026

Signed-off-by: Thomas Mantl <[email protected]>
@TM2500 TM2500 force-pushed the 3026-autosync-disables-sync branch from 789bb51 to 69c3e75 Compare February 16, 2025 12:56
@TM2500 TM2500 marked this pull request as ready for review February 16, 2025 12:57
AnomalRoil
AnomalRoil previously approved these changes Feb 24, 2025
View reviewed changes
Copy link
Member

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

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

LGTM

internal/action/sync.go Outdated
Comment on lines 163 to 164
// using GetM here to get the value for this mount, it might be different
// than the global value
Copy link
Member

Choose a reason for hiding this comment

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

you could have moved the comment inside of the first if I guess

dominikschulz
dominikschulz reviewed Feb 26, 2025
View reviewed changes
Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

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

You are adding a bunch of code just to pass a bool in to the sync method.
Wouldn't it easier to just pass an additional bool to s.sync to signal that it's being invoked from autosync? Or is there something I'm missing?

@TM2500
Copy link
Contributor Author

TM2500 commented Feb 26, 2025

You are adding a bunch of code just to pass a bool in to the sync method. Wouldn't it easier to just pass an additional bool to s.sync to signal that it's being invoked from autosync? Or is there something I'm missing?

You are right, as long as we only depend on the autosync-context only within the sync package, we could just use a function-parameter.
Since there is currently no need for it in other packages, I will rewrite the Patch.

@TM2500
* [bugfix] Don't check for autosync on manual triggered sync
ec782c7 
Refactor to isAutosync function-parameter instead of a new context

Signed-off-by: Ing. Thomas Mantl <[email protected]>
@TM2500
Copy link
Contributor Author

TM2500 commented Feb 26, 2025

I refactored to function arguments.
This is ready to merge.

dominikschulz
dominikschulz approved these changes Feb 27, 2025
View reviewed changes
Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

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

Much simpler. Thanks a lot!

@dominikschulz dominikschulz merged commit 81604c1 into gopasspw:master Feb 27, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominikschulz dominikschulz dominikschulz approved these changes

@AnomalRoil AnomalRoil AnomalRoil left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

autosync = false breaks manual sync
3 participants
@TM2500 @dominikschulz @AnomalRoil