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

Bug/SK-931 | --preferred-combiner should not be boolean #667

Merged
merged 15 commits into from
Aug 16, 2024
Merged

Conversation

sowmyasris
Copy link
Contributor

@sowmyasris sowmyasris commented Aug 1, 2024

Changed --preferred-combiner flag in run_cmd and client_cmd to --combiner
Type change from boolean to string

@github-actions github-actions bot added the minor label Aug 1, 2024
@sowmyasris sowmyasris changed the title Bug/SK-931|--preferred-combiner should not be boolean Bug/SK-931 | --preferred-combiner should not be boolean Aug 1, 2024
@sowmyasris sowmyasris requested a review from Wrede August 1, 2024 08:15
Copy link
Member

@Wrede Wrede left a comment

Choose a reason for hiding this comment

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

I have changed my mind, let keep the name preferred_combiner otherwise it will have conflicts with new feature --combiner and --proxy-host

@@ -38,7 +38,7 @@ class ConnectorClient:
:param force_ssl: True if https is used, False if http
:type force_ssl: bool
:param verify: True if certificate is verified, False if not
:type verify: bool
:type verify: str
Copy link
Member

Choose a reason for hiding this comment

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

why is this changed to "str"?

Copy link
Contributor Author

@sowmyasris sowmyasris Aug 12, 2024

Choose a reason for hiding this comment

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

Because this param is for preferred-combiner which will be the name of the combiner, so verifying if it is string.

@sowmyasris sowmyasris changed the title Bug/SK-931 | --preferred-combiner should not be boolean Bug/SK-931 and SK-931 | --preferred-combiner should not be boolean Aug 12, 2024
@github-actions github-actions bot added the patch label Aug 12, 2024
@sowmyasris sowmyasris requested a review from Wrede August 14, 2024 07:35
@Wrede Wrede changed the title Bug/SK-931 and SK-931 | --preferred-combiner should not be boolean Bug/SK-931 | --preferred-combiner should not be boolean Aug 16, 2024
@@ -47,7 +47,7 @@
"secure": False,
"preshared_cert": False,
"verify": False,
"preferred_combiner": False,
"combiner": "combiner",
Copy link
Member

Choose a reason for hiding this comment

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

you can remove this

@Wrede Wrede merged commit b8b368b into master Aug 16, 2024
19 checks passed
@Wrede Wrede deleted the Bug/SK-931 branch August 16, 2024 11:35
@Wrede Wrede added bugfix Something isn't working and removed github labels Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Something isn't working minor patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants