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: Github Codespaces Plugin #328

Merged
merged 30 commits into from
Apr 15, 2024
Merged

Conversation

pyxelpioneer
Copy link
Contributor

Fixes: amplication/amplication#7012

PR Details

Adds the plugin as per defined in issue #7012

PR Checklist

  • npm build doesn't throw any error
  • npm test doesn't throw any error
  • tested locally

IMPORTANT: Please review the CONTRIBUTING.md file for detailed contributing guidelines.

@overbit
Copy link
Contributor

overbit commented Jan 11, 2024

@pyxelpioneer could you please address the CI failures?

Copy link
Collaborator

@Shurtu-gal Shurtu-gal left a comment

Choose a reason for hiding this comment

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

Nice work 🚀. Just some tiny changes 💯

@pyxelpioneer
Copy link
Contributor Author

@overbit fixed ci failing

Copy link
Collaborator

@Shurtu-gal Shurtu-gal left a comment

Choose a reason for hiding this comment

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

@pyxelpioneer Can you update the README to provide some more context as well as steps on how to startup the dev container after app is generated.

@Shurtu-gal
Copy link
Collaborator

@pyxelpioneer Can you update the README to provide some more context as well as steps on how to startup the dev container after app is generated.

@pyxelpioneer What I meant was adding steps on how to run the dev container after the service is made.

@pyxelpioneer
Copy link
Contributor Author

@pyxelpioneer What I meant was adding steps on how to run the dev container after the service is made.

Oh sorry, I forgot to add that, but now I have added

@Shurtu-gal
Copy link
Collaborator

@overbit rest everything is fine by me. Could you have a look once too?

Copy link
Contributor

@mulygottlieb mulygottlieb left a comment

Choose a reason for hiding this comment

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

Nice work @pyxelpioneer !
I added one comment about an empty file in the tests directory, but other than that it LGTM.

…generate admin ui

This commit lets the admin ui overwrite the devcontainer config during
the `afterCreateAdminUI` event and not in `afterCreateServer` event.
This ensures that if the user doesnt opt in to generate the admin ui,
then the `afterCreateAdminUI` event is never trigged, so admin ui config
doesn't get generated (even if user specifies `includeAdminUI: true` in
settings), thus preventing generating of slate docker compose file
@pyxelpioneer
Copy link
Contributor Author

I am still working, just busy with some other work at the moment. I will upload the video by tomorrow

@pyxelpioneer
Copy link
Contributor Author

Okay so i was busy with intra-state shifting but now I have settled and I will send the video today

@Shurtu-gal
Copy link
Collaborator

@pyxelpioneer any updates on this ?

@pyxelpioneer
Copy link
Contributor Author

@pyxelpioneer any updates on this ?

I was actually making the video, and saw that I was being hit with a CORS error on Admin UI (even though the server and the client are both accessable from web), which I hadn't noticed because I thought it would work out of the box anyways. I don't really know the issue, and I am trying to fix that.

@pyxelpioneer
Copy link
Contributor Author

Just updating, I was able to fix the CORS issue in the build generated by app.amplication.com, but there are still some bugs with the local build (weird). WIll not take too long to fix it up.

@pyxelpioneer
Copy link
Contributor Author

pyxelpioneer commented Apr 7, 2024

Hey @overbit @Shurtu-gal @levivannoort , sorry for the extreme delay due to shifting, my codespace limit gettting full etc. Thanks for not closing this PR!

Here is the video showing the output based on the rework of the plugin.

dance-2024-04-07_22.online-video-cutter.com.mp4

@pyxelpioneer pyxelpioneer requested a review from overbit April 7, 2024 18:03
@pyxelpioneer
Copy link
Contributor Author

Hey @overbit @Shurtu-gal @levivannoort , sorry for the extreme delay due to shifting, my codespace limit gettting full etc. Thanks for not closing this PR!

Here is the video showing the output based on the rework of the plugin.
dance-2024-04-07_22.online-video-cutter.com.mp4

Hey @overbit ?

@overbit
Copy link
Contributor

overbit commented Apr 15, 2024

Well done @pyxelpioneer! We are reviewing it.

@overbit
Copy link
Contributor

overbit commented Apr 15, 2024

@Shurtu-gal are you happy to approve this PR too? I see that all your comments have been addressed

@pyxelpioneer
Copy link
Contributor Author

Well done @pyxelpioneer! We are reviewing it.

Thanks 😍

@Shurtu-gal
Copy link
Collaborator

Great Work @pyxelpioneer!!!

@pyxelpioneer
Copy link
Contributor Author

Great Work @pyxelpioneer!!!

Thanks 😍

@overbit overbit merged commit f9cb44b into amplication:master Apr 15, 2024
4 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.

⭐ Plugin: GitHub Codespaces
5 participants