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

StaticFiles microservice with a files tab #125

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

freshteapot
Copy link
Contributor

Summary

Adding a view screen for static-files-v1 a new microservice kind, it is not yet possible to add this via Studio, but it is possible to interact with it.

@freshteapot freshteapot marked this pull request as draft October 14, 2021 08:20
@freshteapot
Copy link
Contributor Author

freshteapot commented Oct 14, 2021

@joelhoisko joelhoisko marked this pull request as ready for review October 21, 2021 12:40
@joelhoisko joelhoisko self-requested a review October 21, 2021 12:40
Copy link
Contributor

@joelhoisko joelhoisko left a comment

Choose a reason for hiding this comment

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

Good stuff, just found some TS typing stuff. The UI is still to change so I didn't check on that

Comment on lines 19 to 20
const body: any = await response.json() as StaticFiles;
return body;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const body: any = await response.json() as StaticFiles;
return body;
return await response.json();

This is enough for the compiler, as the function already has a Promise<StaticFiles> return type to it.

Comment on lines 23 to 36
export async function addFile(applicationId: string, environment: string, microserviceId: string, fileName: string, file: File): Promise<boolean> {
const url = `${getServerUrlPrefix()}/application/${applicationId}/environment/${environment.toLowerCase()}/staticFiles/${microserviceId}/add/${fileName}`;
const response = await fetch(url, {
method: 'POST',
mode: 'cors',
body: file,
});

if (response.status !== 201) {
console.error(response);
throw Error('Failed to add file');
}
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export async function addFile(applicationId: string, environment: string, microserviceId: string, fileName: string, file: File): Promise<boolean> {
const url = `${getServerUrlPrefix()}/application/${applicationId}/environment/${environment.toLowerCase()}/staticFiles/${microserviceId}/add/${fileName}`;
const response = await fetch(url, {
method: 'POST',
mode: 'cors',
body: file,
});
if (response.status !== 201) {
console.error(response);
throw Error('Failed to add file');
}
return true;
}
export async function addFile(applicationId: string, environment: string, microserviceId: string, fileName: string, file: File): Promise<void> {
const url = `${getServerUrlPrefix()}/application/${applicationId}/environment/${environment.toLowerCase()}/staticFiles/${microserviceId}/add/${fileName}`;
const response = await fetch(url, {
method: 'POST',
mode: 'cors',
body: file,
});
if (response.status !== 201) {
console.error(response);
throw Error('Failed to add file');
}
}

I checked that the return wasn't actually checked anywhere currently so we could just make return type a Promise<void> instead then.

Comment on lines 39 to 53
export async function deleteFile(applicationId: string, environment: string, microserviceId: string, fileName: string): Promise<boolean> {
const url = `${getServerUrlPrefix()}/application/${applicationId}/environment/${environment.toLowerCase()}/staticFiles/${microserviceId}/remove/${fileName}`;
// TODO add file
const response = await fetch(url, {
method: 'DELETE',
mode: 'cors',
});


if (response.status !== 200) {
console.error(response);
throw Error('Failed to delete');
}
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export async function deleteFile(applicationId: string, environment: string, microserviceId: string, fileName: string): Promise<boolean> {
const url = `${getServerUrlPrefix()}/application/${applicationId}/environment/${environment.toLowerCase()}/staticFiles/${microserviceId}/remove/${fileName}`;
// TODO add file
const response = await fetch(url, {
method: 'DELETE',
mode: 'cors',
});
if (response.status !== 200) {
console.error(response);
throw Error('Failed to delete');
}
return true;
}
export async function deleteFile(applicationId: string, environment: string, microserviceId: string, fileName: string): Promise<void> {
const url = `${getServerUrlPrefix()}/application/${applicationId}/environment/${environment.toLowerCase()}/staticFiles/${microserviceId}/remove/${fileName}`;
// TODO add file
const response = await fetch(url, {
method: 'DELETE',
mode: 'cors',
});
if (response.status !== 200) {
console.error(response);
throw Error('Failed to delete');
}
}

Same here, I checked that the return wasn't actually checked anywhere currently so we could just make return type Promise<void> instead then.

const data = _props.data;
const cdnInfo = _props.cdnInfo;

const items: ListItem[] = data.files.map<ListItem>(e => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const items: ListItem[] = data.files.map<ListItem>(e => {
const items = data.files.map<ListItem>(e => {

The compilers smart enough here if you just define it in the map<ListItem> type argument to know the resulting type for items

Comment on lines +23 to +29
// TODO this should not be hardcoded
// TODO Make sure we remove trailing slash
const cdnInfo = {
domain: 'https://freshteapot-taco.dolittle.cloud',
prefix: '/doc/',
path: '',
} as CdnInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we need to fix this before going to production.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to move this, to the api response from the platform-api.
Then the Studio is free and the data is correct and from the right place.
Good spot.

const microserviceId = _props.microserviceId;
const environment = _props.environment;

const [selectedFile, setSelectedFile] = React.useState(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [selectedFile, setSelectedFile] = React.useState(null);
const [selectedFile, setSelectedFile] = React.useState<File>();

Can use this so that you don't have to later cast it to a File

let suffix = fileName.replace(cdnInfo.path, '');
suffix = suffix.startsWith('/') ? suffix.substring(1) : suffix;

await addFile(applicationId, environment, microserviceId, suffix, selectedFile! as File);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await addFile(applicationId, environment, microserviceId, suffix, selectedFile! as File);
await addFile(applicationId, environment, microserviceId, suffix, selectedFile!);

Can remove the as File part if you set it in the useState<File> from earlier.

const [reset, setReset] = React.useState(false);

const [runtimeError, setRuntimeError] = React.useState(null as any);
const [currentFiles, setCurrentFiles] = useState({ files: [] } as StaticFiles);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [currentFiles, setCurrentFiles] = useState({ files: [] } as StaticFiles);
const [currentFiles, setCurrentFiles] = useState<StaticFiles>({ files: [] });

Same trick here.

Comment on lines 97 to 98
// TODO modify when we know how we want to handle state of purchase order data
// Fake it till we are ready
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// TODO modify when we know how we want to handle state of purchase order data
// Fake it till we are ready

Some old copy pasta slipping in 😂

currentMicroservice.live.images[0] &&
currentMicroservice.live.images[0].image &&
currentMicroservice.live.images[0].image === '453e04a74f9d42f2b36cd51fa2c83fa3.azurecr.io/dolittle/platform/platform-api:dev-x'
currentMicroservice?.live?.images[0]?.image === '453e04a74f9d42f2b36cd51fa2c83fa3.azurecr.io/dolittle/platform/platform-api:dev-x'
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to keep learning from you :)

freshteapot added 2 commits October 22, 2021 08:30
- Upload hanlder shows error if file not selected
- TODO about removing logic pinned to an out of date image
@jakhog jakhog marked this pull request as draft April 13, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants