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

Fix: Added Rebuild button for function view when build fails during deployment #376

Conversation

singhbhaskar
Copy link
Contributor

@singhbhaskar singhbhaskar commented Apr 1, 2023

What does this PR do?

Adds a new button to rebuild function deployment if it fails. The button is not showed when the build is successfull.

Test Plan

Mannual testing. Ran npm run build, npm run format and npm run lint. Attaching the screenshot for the following test.

Screenshot for function failure:

rebuildButtonFromOptionMenu

Screenshot for Model Alert when user clicks on rebuild button:

ModelAlertToConfirmRebuild

Screenshot for Notification alert when rebuild request is process by the SDK:

NotificationAfterRebuildRequestSuccessfullyHitsTheBackend

Screenshot for Successful function build which doesn't show the rebuild option.

rebuildButtonNotShowingForSuccessfulFunctionDeployment

Related PRs and Issues

appwrite/appwrite#4840

Have you read the Contributing Guidelines on issues?

Yes I have read the contributing guidelines.

@vercel
Copy link

vercel bot commented Apr 1, 2023

@singhbhaskar is attempting to deploy a commit to the appwrite Team on Vercel.

A member of the Team first needs to authorize it.

@stnguyen90 stnguyen90 requested review from TorstenDittmann and stnguyen90 and removed request for TorstenDittmann April 6, 2023 16:23
Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

This looks great! Good idea on the confirmation modal too! I just have some minor text suggestions.

@singhbhaskar
Copy link
Contributor Author

This looks great! Good idea on the confirmation modal too! I just have some minor text suggestions.

Sure Steven, I'll update the suggest changes. Thanks

@singhbhaskar singhbhaskar force-pushed the feat-4840-allow-retrying-failed-builds branch from 75cca9d to 2120d77 Compare April 16, 2023 10:47
@singhbhaskar
Copy link
Contributor Author

Hi @stnguyen90 ,
I have addressed all the comments. Kindly review again. Attaching the latest screenshots.

Screenshot for function failure:

retryBuildOptions

Screenshot for Model Alert when user clicks on rebuild button:

retryBuildModel

Screenshot for Notification alert when rebuild request is process by the SDK:

retryBuildSucess

@stnguyen90
Copy link
Contributor

@singhbhaskar looks like the tests are failing. Perhaps the package name has changed?

Also, please make sure to re-request a review when you're ready for a review.

@aw-labs/appwrite-console to
@appwrite.io/console for rebuild.svelte
@singhbhaskar singhbhaskar requested a review from stnguyen90 April 21, 2023 17:00
@singhbhaskar
Copy link
Contributor Author

Hi @stnguyen90 ,
Updated the code to fix test. Please review.

@singhbhaskar singhbhaskar requested a review from stnguyen90 April 26, 2023 18:16
@singhbhaskar
Copy link
Contributor Author

I am able to successfully run lint and test in my local system but for some reason it's failing during the check. Can you trigger a rebuild?

@stnguyen90
Copy link
Contributor

I am able to successfully run lint and test in my local system but for some reason it's failing during the check. Can you trigger a rebuild?

@TorstenDittmann, I also tried to check the linting and it seems fine.

I manually ran the tests and they all passed:

image

I can confirm the modal triggers a new build:

image

What do you think about just merging?

@stnguyen90 stnguyen90 requested a review from TorstenDittmann May 3, 2023 00:10
@singhbhaskar singhbhaskar requested a review from stnguyen90 May 4, 2023 15:51
Copy link
Contributor

@TorstenDittmann TorstenDittmann left a comment

Choose a reason for hiding this comment

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

once you have fixed the formatting issues we can merge 👍🏻

just run npm run format

@singhbhaskar
Copy link
Contributor Author

npm run format

Hi,
I had already ran npm run format when raising the PR. I re-ran it just now and there is no file format change for any file. Can you tell which file is having format issue?

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.

3 participants