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

Providers schema #53

Merged
merged 22 commits into from
Mar 29, 2023
Merged

Providers schema #53

merged 22 commits into from
Mar 29, 2023

Conversation

johncalesp
Copy link
Contributor

  • Added a new setting so users can reference external JSON files about cloud instances
  • Added new JSON schema for cloud providers
  • Change references to pull data from centML S3 bucket
  • Changed README file to let users know about our JSON schema

code: `status: ${resp.statusText} | code: ${resp.status}`,
});
}
} else {
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

this.errors = [];
}
}
export const loadJsonFiles = async (habitatData, additionalProviders) => {
Copy link

Choose a reason for hiding this comment

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

Function loadJsonFiles has 77 lines of code (exceeds 25 allowed). Consider refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could refactor by wrapping some repeated logic with helper functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved logic to utils.js

logo: cloudProvider.logo,
color: cloudProvider.color,
};
for (const instanceData of cloudProvider.instances) {
Copy link

Choose a reason for hiding this comment

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

Avoid deeply nested control flow statements.

<DeploymentTab numIterations={numIterations} habitatData={data} />
);
// Mock fetch response
jest.spyOn(global, 'fetch').mockResolvedValue({
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 3 locations. Consider refactoring.

// ARRANGE
// Mock fetch response
jest.spyOn(global, 'fetch').mockResolvedValue({
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 3 locations. Consider refactoring.


const { ResizeObserver } = window;
const numIterations = 10000;

const data = [
const habitatData = [
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

// ARRANGE
const { container } = render(<ProviderPanel numIterations={numIterations} habitatData={noHabitatData} />);
// Mock fetch response
jest.spyOn(global, 'fetch').mockResolvedValue({
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 3 locations. Consider refactoring.

["RTX4000", 20.2342],
];

const correctData = [
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

@@ -19,6 +21,35 @@ const data = [
["RTX4000", 20.2342],
];

const correctData = [
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

})),
});
}
} catch (error) {
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could encapsulate this into a helper function

README.md Outdated
@@ -29,14 +29,44 @@ To install, either:
Once you have the vsix file, run `code --install-extension vscode*.vsix` to install the extension.
**Note: the file [build_vsix_dev.sh] is only to be used for development**

**Adding cloud instances to the Deployment Tab:** You can include information about the instances that you use thrugh the extension settings. There you will find an option named **providers** that accepts a list of urls separated by commas. Each url must be a JSON file that follows the schema speficied here : [schema](skyline-vscode/react-ui/src/schema/CloudProvidersSchema.js).<br/>
Copy link
Contributor

Choose a reason for hiding this comment

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

typo on "specified"

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

README.md Outdated
@@ -29,14 +29,44 @@ To install, either:
Once you have the vsix file, run `code --install-extension vscode*.vsix` to install the extension.
**Note: the file [build_vsix_dev.sh] is only to be used for development**

**Adding cloud instances to the Deployment Tab:** You can include information about the instances that you use thrugh the extension settings. There you will find an option named **providers** that accepts a list of urls separated by commas. Each url must be a JSON file that follows the schema speficied here : [schema](skyline-vscode/react-ui/src/schema/CloudProvidersSchema.js).<br/>
Addittionally, you need to add the necessary access so the extension can read the file.<br/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo on Additionally

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

export const loadJsonFiles = async (habitatData, additionalProviders) => {
const buffer = new ResponseBuffer();
let urlList = [
"https://deepview-explorer-public.s3.amazonaws.com/vscode-cloud-providers/providers.json",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be stored as a constant in properties.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved variable to properties.js

const ajv = new Ajv({ allErrors: true }); // to report all validation errors (rather than failing on the first errors)
const validate = ajv.compile(cloudProviderSchema);

class ResponseBuffer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this so it is more clear what is used for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

class was deleted

})),
});
}
} catch (error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could encapsulate this into a helper function

this.errors = [];
}
}
export const loadJsonFiles = async (habitatData, additionalProviders) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could refactor by wrapping some repeated logic with helper functions

const ajv = new Ajv({ allErrors: true }); // to report all validation errors (rather than failing on the first errors)
const validate = ajv.compile(cloudProviderSchema);

export const loadJsonFiles = async (habitatData, additionalProviders) => {
Copy link

Choose a reason for hiding this comment

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

Function loadJsonFiles has 68 lines of code (exceeds 25 allowed). Consider refactoring.

fill: "#ea4335",
z: 200,
});
expect(resp.instanceArray).toContainEqual({
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

import { enableFetchMocks } from "jest-fetch-mock";
enableFetchMocks();

const habitatData = [
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

},
});
expect(resp.instanceArray.length).toEqual(2);
expect(resp.instanceArray).toContainEqual({
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

json: jest.fn().mockResolvedValue(correctData)
})

const { container } = render(
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

@johncalesp johncalesp requested a review from michaelshin March 29, 2023 15:55
}

export function fetchingURLErrors(type, response, validationErrors) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function should function is doing too much specific work and not general work. I think you should keep the general case. For the invalid schema error, you can revert that change and have this only handle errors when fetching URLs.

Also I think you should remain the function to the active voice (e.g. getErrMsgFromInvalidURL

@johncalesp johncalesp requested a review from michaelshin March 29, 2023 16:32
const ajv = new Ajv({ allErrors: true }); // to report all validation errors (rather than failing on the first errors)
const validate = ajv.compile(cloudProviderSchema);

export const loadJsonFiles = async (habitatData, additionalProviders) => {
Copy link

Choose a reason for hiding this comment

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

Function loadJsonFiles has 74 lines of code (exceeds 25 allowed). Consider refactoring.

ok: true,
json: jest.fn().mockResolvedValue(correctData)
})
const { container } = render(<ProviderPanel numIterations={numIterations} habitatData={noHabitatData} additionalProviders={""}/>);
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

const ajv = new Ajv({ allErrors: true }); // to report all validation errors (rather than failing on the first errors)
const validate = ajv.compile(cloudProviderSchema);

export const loadJsonFiles = async (habitatData, additionalProviders) => {
Copy link

Choose a reason for hiding this comment

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

Function loadJsonFiles has a Cognitive Complexity of 24 (exceeds 5 allowed). Consider refactoring.

];

test("Validate JSON file (URL) and return list of cloud providers and instances", async () => {
jest.spyOn(global, "fetch").mockResolvedValue({
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

});

test("JSON file (URL) is not in the correct format", async () => {
jest.spyOn(global, "fetch").mockResolvedValue({
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Mar 29, 2023

Code Climate has analyzed commit 2349588 and detected 19 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 4
Duplication 15

View more on Code Climate.

@johncalesp johncalesp merged commit 0d47046 into main Mar 29, 2023
@johncalesp johncalesp deleted the providers-schema branch March 29, 2023 16:43
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