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

Add SIGV4 wrapper component #17

Merged
merged 4 commits into from
May 19, 2022
Merged

Add SIGV4 wrapper component #17

merged 4 commits into from
May 19, 2022

Conversation

sunker
Copy link
Contributor

@sunker sunker commented May 16, 2022

Related to grafana/grafana#43559

This PR moves the SIGV4ConnectionConfig from grafana to this repo where it belongs. The SIGV4ConnectionConfig is basically a wrapper around the ConnectionConfig component, making sure prop names are mapped properly. Unfortunately sigv4 props are not types in core grafana, hence all the tests for the mapping of the props.

@sunker sunker requested a review from andresmgot May 16, 2022 10:16
@sunker sunker requested review from a team, fridgepoet, iwysiu and sarahzinger and removed request for a team, andresmgot and fridgepoet May 17, 2022 07:50
@sunker sunker changed the title Sigv4 component Add SIGV4 wrapper component May 18, 2022
Copy link
Member

@sarahzinger sarahzinger left a comment

Choose a reason for hiding this comment

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

🎉 cool stuff, I didn't do any manual testing since I wasn't entirely sure how to but I feel like conceptually it makes sense! I wrote some little code-style suggestions but feel free to ignore if you disagree!

import { AwsAuthType } from 'types';

const resetWindow = () => {
(window as any).grafanaBootData = {
Copy link
Member

Choose a reason for hiding this comment

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

I suspect in this case it does not matter however a pattern I have seen done for this kind of thing before is to do something like:

const old = window.grafanaBootData
// everything you have already
afterEach(() => window.grafanaBootData = old)

I think otherwise there might be a possibility that in some other test you might be testing that window.grafanaBootData gets attached to window properly and it will fail or give you a false positive because it's carrying over the global window object between test files.

Kind of nitpicky but maybe worth it? I'm not terribly familiar with this property so I don't really know.

Oh also just remembering that I think jest might have some tooling for this: https://stackoverflow.com/questions/41885841/how-can-i-mock-the-javascript-window-object-using-jest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const old = window.grafanaBootData
// everything you have already
afterEach(() => window.grafanaBootData = old)

^ I think this pattern is definitely preferable in the case window.grafanaBootData existed, but the window object doesn't exist in the jest context so it needs to be created before each test.

However, this made me realize one thing. The reason grafanaBootData was taken from the window object and not from grafana/runtime was that until grafana/grafana#43559 is merged, adding grafana/runtime to the aws-sdk would created an implicit dependency from grafana/ui to grafana/runtime which is not good. Now that the dep from grafana/ui to aws-sdk is removed, aws-sdk should be able to depend on grafana/runtime again. :) I'll do that in an another PR for tractability. Also then I should be able to refactor these tests and mock config from grafana/runtime instead of mocking the whole window object. Thanks!


it('should map changed fields correctly', async () => {
const onOptionsChange = jest.fn();
setup(onOptionsChange);
Copy link
Member

Choose a reason for hiding this comment

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

I think I have a preference here to do something like this instead of setup():

it('should map incoming props correctly', () => {
  const props = createDefaultProps() <-- the same as your setup call but you return props rather than have a side-effect of rendering
  render(<SIGV4ConnectionConfig {...props} />);
///whatever else the same
// and in tests when you need to override onOptionsChange
  const props = createDefaultProps();
  const onOptionsChange = jest.fn()
  render(<SIGV4ConnectionConfig {...props} onOptionsChange={onOptionsChange} />);

I just find it slightly easier to read, but again very nitpicky, please feel free to ignore if you disagree haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use this pattern in various places across Grafana. I agree your approach is more readable, but still think this is fine in case the test doesn't need to have custom props. Will leave it as is for now.

export const SIGV4ConnectionConfig: React.FC<DataSourcePluginOptionsEditorProps<any, any>> = (
props: DataSourcePluginOptionsEditorProps<any, any>
) => {
console.log('sigv4', props);
Copy link
Member

Choose a reason for hiding this comment

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

oops


import { AwsAuthDataSourceSecureJsonData, AwsAuthDataSourceJsonData } from './types';

export const SIGV4ConnectionConfig: React.FC<DataSourcePluginOptionsEditorProps<any, any>> = (
Copy link
Member

Choose a reason for hiding this comment

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

😭 no way to type this i guess right? Would unknown be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not since HttpSettingsProps are types as any in core grafana. :( See https://github.com/grafana/grafana/blob/main/packages/grafana-ui/src/components/DataSourceSettings/types.ts#L22

sigV4Region: 'us-east-2',
timeField: '@timestamp',
},
secureJsonFields: { sigV4AccessKey: true, sigV4SecretKey: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

should these be filler strings instead of filler bools?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For security reasons grafana backend doesn't pass the secure field as text to the frontend. It just passes a boolean value for each field to indicate weather it's set or not.

@sunker sunker merged commit ef9ecf4 into main May 19, 2022
@sunker sunker deleted the sigv4-component branch May 19, 2022 09:18
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

3 participants