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

feat: add support for disabling locking with NoOp lock provider #278

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

duoctranth
Copy link
Contributor

This pull request introduces the ability to disable locking functionality by adding a new action input called disable-locking. When set to true, it utilizes the NoOp lock provider, which performs no actual locking operations.

The logic code has been implemented to support this new feature, ensuring the appropriate environment is utilized based on the disable-locking input value.

For more details, please refer to the linked issue.

Linked Issue: #99

@@ -77,7 +92,9 @@ func (projectLock *ProjectLockImpl) Lock(prNumber int) (bool, error) {
return false, err
}

if lockAcquired {
_, isNoOpLock := projectLock.InternalLock.(*NoOpLock)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this kinda defeats the purpose of having polymorphic lock if we need to do a type check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback. Regarding the NoOpLock, would it be possible to add an additional return parameter called lockIgnored to the Lock interface instead of using this alternative approach?
I'm open to exploring other ideas as well. Please let me know if you have any suggestions or recommendations.

@Spartakovic
Copy link
Contributor

I wonder if it wouldn't be more straightforward instead of having no-op lock, just make a decision about locking on DiggerExecutor level.
No-op locking and type checking a level higher look more like workarounds

@duoctranth
Copy link
Contributor Author

duoctranth commented May 20, 2023

I wonder if it wouldn't be more straightforward instead of having no-op lock, just make a decision about locking on DiggerExecutor level. No-op locking and type checking a level higher look more like workarounds

Thank you for your suggestion. I understand your point about making the decision about locking on the DiggerExecutor level instead of using a no-op lock. While it's true that using no-op locking and type checking at a higher level can be seen as workarounds, the reason we opted for this approach is to keep the code changes minimal and provide the flexibility to customize the locking behavior in the future if needed.

By using the no-op lock, we can easily switch between enabling and disabling locking without modifying the DiggerExecutor logic. This allows us to maintain a clear separation between the locking mechanism and the executor logic. Additionally, it provides an opportunity for customization by allowing different messages to be displayed later on.

However, I appreciate your input, and if you think there is a more straightforward approach that aligns with the project's goals and architecture, I'm open to discussing it further. Let's explore all options and come up with the best solution together. Feel free to share any specific ideas or concerns you have regarding the locking implementation.

@motatoes motatoes merged commit d918cbf into diggerhq:develop Jun 1, 2023
motatoes added a commit that referenced this pull request Jun 1, 2023
* Update CONTRIBUTING.md (#303)

* fix(deps): update module github.com/xanzy/go-gitlab to v0.84.0 (#306)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update module github.com/stretchr/testify to v1.8.4 (#302)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update module github.com/aws/aws-sdk-go to v1.44.268 (#290)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* feat: Add support for NoOp lock implementation (#278)

Co-authored-by: duoctt <duoctt@vng.com.vn>

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Duoc Tran <51254650+duoctranth@users.noreply.github.com>
Co-authored-by: duoctt <duoctt@vng.com.vn>
motatoes added a commit that referenced this pull request May 16, 2024
* Update CONTRIBUTING.md (#303)

* fix(deps): update module github.com/xanzy/go-gitlab to v0.84.0 (#306)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update module github.com/stretchr/testify to v1.8.4 (#302)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(deps): update module github.com/aws/aws-sdk-go to v1.44.268 (#290)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* feat: Add support for NoOp lock implementation (#278)

Co-authored-by: duoctt <duoctt@vng.com.vn>

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Duoc Tran <51254650+duoctranth@users.noreply.github.com>
Co-authored-by: duoctt <duoctt@vng.com.vn>
ben-of-codecraft pushed a commit to ben-of-codecraft/digger that referenced this pull request May 21, 2024
Co-authored-by: duoctt <duoctt@vng.com.vn>
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