-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat: merge "Upload SDL" to "Build your template" and add "Plain Linux" template #244
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hiroyukikumazawa Thanks a bunch for your contribution!
I left some comments. Some of them may be considered a refactoring so please do reply if you feel comfortable fulfilling those.
While testing I also spotted a couple of bugs.
- it is only possible to upload an SDL if it's not set yet. E.g. if the builder tab was visited and thus SDL is defined or if it is already defined in a different way uploaded SDL has no effect
- if input is cancelled and no SDL is uploaded the implemented handler still tries to process the event and throws
I recorded a video with these for you.
@hiroyukikumazawa We usually add this into commits. E.g.
|
@hiroyukikumazawa another thing I noticed is it seems you haven't used our linter and formatter. We're going to add that as a pre-commit hook. However in the meanwhile please run before commit/push: npm run lint -- --fix
npm run format I'd suggest you enable that in your IDE, should ease the whole thing generally. |
So many files have been changed even though that I didn't change. @ygrishajev |
@hiroyukikumazawa That must me EOLs. If so I think u should be safe to commit. This don't appear on diff eventually. |
Right, so I will commit the changes. |
Yea, just make a separate commit for the review. I'll check if anything's off. |
Ok |
@hiroyukikumazawa That commit looks good to me. It's a lot of changes indeed but this has to be done anyway. Thanks for doing this. Let's see if others are aligned on leaving it in this pr. |
apps/deploy-web/src/components/new-deployment/NewDeploymentContainer.tsx
Outdated
Show resolved
Hide resolved
"use client"; | ||
|
||
export const PlainVMForm: React.FunctionComponent = () => { | ||
return <>Comming Soon</>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anilmurty Do you want to release it like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's going on the main deploy page, then I think no. This task was to just merge the upload SDL into the "create your template" tile. The VM tile can be done separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so should we set this thread to resolved status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have commented out the "Plain Linux" tile for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hiroyukikumazawa thanks for bearing with us! Added couple comments :)
apps/deploy-web/src/components/new-deployment/NewDeploymentContainer.tsx
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/components/new-deployment/NewDeploymentContainer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple more. Apologise if these are not ur changes, just let me know and resolve please
We may have to comment out the "Plain Linux" tile for now. Otherwise LGTM 👍 |
Please squash commits before merging. Would be nice if we could keep formatting separately though |
040bb57
to
1defcd1
Compare
done squash |
@hiroyukikumazawa lgtm! thanks again for ur contribution! good job 💪 |
@hiroyukikumazawa for your future contributions pls try using rebase instead of merge. Let's keep it as is for now though :) |
- Improved type safety - Allowed re-selection of the same SDL file by clearing input value - Changed mode to YAML when uploading SDL - Removed 'Upload SDL' button in other templates - Updated coding style - Improved coding style and renamed functions for better readability - Ran npm run lint -- --fix and npm run format
51f0747
to
dbc6e21
Compare
done, please check again |
dbc6e21
to
1f7ea41
Compare
refs #227