Skip to content

[java] set isDisabled to true by default (make Bindings opt-in) #342

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

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

titusfortner
Copy link
Collaborator

I think we want to make Sauce Bindings opt-in instead of opt-out.

This has the advantage of not changing default behavior when registering extension or adding rule.
This would require the user to update their CI tool to run with an environment variable or system property.
This would require the user to add static { System.setProperty("sauce.disabled", "false") } in order to run tests on Sauce Labs.
If we have this logic in CI plugins (e.g. Jenkins), it would just work on CI. If we create IDE plugins, we just add the toggle to the Run configs or context clicks.

Now that we are making this switch, do we want to make this sauce.enabled instead? Probably.

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Yeah, I would change the name to enabled.

@titusfortner
Copy link
Collaborator Author

Ok, so part of making things opt-in...
I switched from maven executions to profiles and updated the Github actions workflows accordingly.

I added the convenience method for enabling the extension rather than requiring a system property, but added the details in the code comments.

Really when it comes down to it, I want to make sure that these examples are what we want users to do:
https://github.com/saucelabs/sauce_bindings/tree/513cd55a16b33c73e104b9011bdef05075518371/java/junit5/src/test/java/com/saucelabs/saucebindings/junit5/examples

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

I'd say this is good enough.

@titusfortner titusfortner merged commit 13d0408 into main Nov 6, 2024
10 checks passed
@titusfortner titusfortner deleted the opt-in branch November 6, 2024 22:57
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