Skip to content

Conversation

harshmange44
Copy link
Contributor

@harshmange44 harshmange44 commented Apr 28, 2023

Description

  1. Optional SSL support for Redshift plugin, 2. The default value would be 'DEFAULT' -> default config

Fixes #22155

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

Dev activity

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • PR is being merged under a feature flag

QA activity:

  • Test plan has been approved by relevant developers
  • Test plan has been peer reviewed by QA
  • Cypress test cases have been added and approved by either SDET or manual QA
  • Organized project review call with relevant stakeholders after Round 1/2 of QA
  • Added Test Plan Approved label after reveiwing all Cypress test

@rohan-arthur rohan-arthur requested a review from sumitsum May 1, 2023 10:15
{
"label": "Default",
"value": "DEFAULT"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove:
(1) "hidden": true
This should make the SSL dropdown visible on the Redshift datasource config page.
(2) Options that do not apply e.g. verify-ca, verify-full

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this @sumitsum

{
"label": "Default",
"value": "DEFAULT"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a small screen recording in the description showing how this looks and works ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will try to do it in a day or two. Currently facing issues due to lack of enough resources. I saw some of the similar plugins having this <label, value> in form.json

@sumitsum
Copy link
Contributor

Hi @harshmange44 are you still working on this ?

@harshmange44
Copy link
Contributor Author

Hi @harshmange44 are you still working on this ?

Yes @sumitsum, regarding your query about the 'DEFAULT' config in form.json. I noticed one pattern, the postgres plugin also has the same 'DEFAULT' config (MySQL plugin recent changes as well). Shall we remove 'DEFAULT' from the Redshift plugin? Could you pls guide me here?

@harshmange44
Copy link
Contributor Author

Screenshot 2023-05-23 at 12 05 34 PM Screenshot 2023-05-23 at 12 05 19 PM

@sumitsum sumitsum added the Community Contributor Meant to track issues that are assigned to external contributors label Jun 7, 2023
@sumitsum
Copy link
Contributor

sumitsum commented Jun 20, 2023

Hi @harshmange44 are you still working on this ?

Yes @sumitsum, regarding your query about the 'DEFAULT' config in form.json. I noticed one pattern, the postgres plugin also has the same 'DEFAULT' config (MySQL plugin recent changes as well). Shall we remove 'DEFAULT' from the Redshift plugin? Could you pls guide me here?

Hi @harshmange44 , please let the default option stay. Default option means doing nothing from our side and leaving the config to the default driver behaviour.
Also, when you are done with all the changes, can you please create a video to show how it works ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contributor Meant to track issues that are assigned to external contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Redshift to have SSL optional
2 participants