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

ConnectionConfig: update input width for experimental auth component #52

Merged
merged 2 commits into from
Aug 4, 2023

Conversation

bohandley
Copy link
Contributor

@bohandley bohandley commented Jul 27, 2023

What is this?
This is a small UI change, an update to the input widths for the connection config component, only when it is inside the experimental auth component.

Why is this needed?
The experimental auth component is part of the Config Overhaul project and the Prometheus config has recently been updated to use the new auth component.

Here is what it looks like without the update:
Screenshot 2023-07-26 at 8 56 23 AM

Here is what it looks like with the update:
Screenshot 2023-07-26 at 11 51 28 AM

Notes for the reviewers:
Let me know if you have any questions or have any suggestions!

@bohandley bohandley requested a review from a team as a code owner July 27, 2023 19:20
@bohandley bohandley requested review from idastambuk and kevinwcyu and removed request for a team July 27, 2023 19:20
@CLAassistant
Copy link

CLAassistant commented Jul 27, 2023

CLA assistant check
All committers have signed the CLA.

@idastambuk
Copy link
Contributor

Looks good with the grafana/aws-sdk part, but I see this in e.g. Athena:

Screenshot 2023-07-28 at 10 51 36 AM

with the rest of the form with the old width.

Most of our datasources have some kind of datasource-specific config fields, so I guess the datasources would have to all be updated at some point?

@@ -71,6 +71,10 @@ export const ConnectionConfig: FC<ConnectionConfigProps> = (props: ConnectionCon
loadRegions().then((regions) => setRegions(regions.map(toOption)));
}, [loadRegions]);

const inExperimentalAuthComponent = options.jsonData.inExperimentalAuthComponent;
Copy link
Member

Choose a reason for hiding this comment

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

Just curious why we would want to store this in options.jsonData vs adding this as a prop on <ConnectionConfig inExperimentalAuthComponent={true} >

I think the downside to storing this in jsonData is then you need to update all the datasource instances right? Maybe easier to add a temporary flag in the javascript rather than update the user's stored datasource configuration, yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarahzinger
Good point! I was having trouble adding the prop to the component with regards to eliminating type errors so I chose to add the flag in the json data, which is not good to update.

For prometheus, we would have to pass the flag into two components, first the SigV4ConnectionConfig and then again into the ConnectionConfig. I can explore doing that and fixing any typing issues that appear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarahzinger Added as a prop now!

@bohandley
Copy link
Contributor Author

@idastambuk, for the Athena issue, this is a good point!

Most of our datasources have some kind of datasource-specific config fields, so I guess the datasources would have to all be updated at some point?

As mentioned above, Prometheus only uses the SigV4ConnectionConfig but I am not sure where the Athena details are defined or what component those details are in. Is the Athena component another wrapper for the connection config used in other data sources?

@idastambuk
Copy link
Contributor

@idastambuk, for the Athena issue, this is a good point!

Most of our datasources have some kind of datasource-specific config fields, so I guess the datasources would have to all be updated at some point?

As mentioned above, Prometheus only uses the SigV4ConnectionConfig but I am not sure where the Athena details are defined or what component those details are in. Is the Athena component another wrapper for the connection config used in other data sources?

Sorry @bohandley, I should probably have used a better example than Athena!
We only currently have sigv4 option in OpenSearch, and at some point this flow will be used for that too I assume. I guess we will just have to adjust the width when we start updating our forms with the new auth, so disregard this!

Just to answer your question about config forms in Athena (and all other datasources I guess):
Exactly, we have the as part of all of them, but theres datasource-specific inputs that are added to the form in each repo's ConfigEditor component.

@bohandley bohandley merged commit eb55870 into main Aug 4, 2023
3 checks passed
@bohandley bohandley deleted the bohandley/update-auth-width branch August 4, 2023 14:09
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.

None yet

4 participants