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

feat: merge "Upload SDL" to "Build your template" and add "Plain Linux" template #244

Merged
merged 5 commits into from
Jun 21, 2024

Conversation

hiroyukikumazawa
Copy link
Contributor

@hiroyukikumazawa hiroyukikumazawa commented Jun 19, 2024

refs #227

Copy link
Contributor

@ygrishajev ygrishajev left a 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.

  1. 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
  2. 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.

apps/deploy-web/src/components/sdl/PlainLinuxForm.tsx Outdated Show resolved Hide resolved
@ygrishajev
Copy link
Contributor

ygrishajev commented Jun 19, 2024

For issue #227

@hiroyukikumazawa We usually add this into commits. E.g.

fix: remove 'Upload SDL' button in other templates

refs #227

@ygrishajev
Copy link
Contributor

ygrishajev commented Jun 19, 2024

@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.

@hiroyukikumazawa
Copy link
Contributor Author

hiroyukikumazawa commented Jun 19, 2024

@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

@ygrishajev
Copy link
Contributor

@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.

@hiroyukikumazawa
Copy link
Contributor Author

That must me EOLs. If so I think u should be safe to commit. This don't appear on diff eventually

image

There are a lot of these changes.

@ygrishajev
Copy link
Contributor

That must me EOLs. If so I think u should be safe to commit. This don't appear on diff eventually

image

There are a lot of these changes.

Ah, changes like these are fine. There's eslint rule to sort imports. Someone else must have not used it before u

@hiroyukikumazawa
Copy link
Contributor Author

There are a lot of these changes.

Ah, changes like these are fine. There's eslint rule to sort imports. Someone else must have not used it before u

Right, so I will commit the changes.
Are you ok?

@ygrishajev
Copy link
Contributor

There are a lot of these changes.

Ah, changes like these are fine. There's eslint rule to sort imports. Someone else must have not used it before u

Right, so I will commit the changes.

Are you ok?

Yea, just make a separate commit for the review. I'll check if anything's off.

@hiroyukikumazawa
Copy link
Contributor Author

There are a lot of these changes.

Ah, changes like these are fine. There's eslint rule to sort imports. Someone else must have not used it before u

Right, so I will commit the changes.
Are you ok?

Yea, just make a separate commit for the review. I'll check if anything's off.

Ok

@ygrishajev
Copy link
Contributor

@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.

"use client";

export const PlainVMForm: React.FunctionComponent = () => {
return <>Comming Soon</>;
Copy link
Contributor

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?

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

Copy link
Contributor Author

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.

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 have commented out the "Plain Linux" tile for now.

Copy link
Contributor

@ygrishajev ygrishajev left a 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 :)

Copy link
Contributor

@ygrishajev ygrishajev left a 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

@baktun14
Copy link
Contributor

baktun14 commented Jun 20, 2024

We may have to comment out the "Plain Linux" tile for now. Otherwise LGTM 👍

@ygrishajev
Copy link
Contributor

Please squash commits before merging. Would be nice if we could keep formatting separately though

@hiroyukikumazawa
Copy link
Contributor Author

hiroyukikumazawa commented Jun 21, 2024

Please squash commits before merging. Would be nice if we could keep formatting separately though

done squash
please check again. @ygrishajev

@ygrishajev
Copy link
Contributor

Please squash commits before merging. Would be nice if we could keep formatting separately though

done squash please check again. @ygrishajev

@hiroyukikumazawa lgtm! thanks again for ur contribution! good job 💪

@ygrishajev
Copy link
Contributor

@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
@hiroyukikumazawa
Copy link
Contributor Author

@hiroyukikumazawa for your future contributions pls try using rebase instead of merge. Let's keep it as is for now though :)

done, please check again

@baktun14 baktun14 merged commit 0edf499 into akash-network:main Jun 21, 2024
6 checks passed
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.

4 participants