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(expressions): add router-playground-modal [KM-299] #1497

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

2eha0
Copy link
Contributor

@2eha0 2eha0 commented Jul 15, 2024

Summary

  • migrate <RouterPlaygroundModal /> from kong-admin to public-ui
  • bump it in <RouterForm /> and add some tests
  • RouterPlayground.cy.ts is skipped for temporary, it requires wasm and Monaco vite plugins during the component tests running, which we should discuss with core UI team

@CLAassistant
Copy link

CLAassistant commented Jul 15, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Zehao Zhang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@2eha0 2eha0 force-pushed the feat/KM-299-router-playground branch 2 times, most recently from 76470af to f26aaff Compare July 15, 2024 09:41
@2eha0 2eha0 force-pushed the feat/KM-299-router-playground branch from f26aaff to 171f98f Compare July 15, 2024 10:07
@2eha0 2eha0 force-pushed the feat/KM-299-router-playground branch 7 times, most recently from c16d749 to ff6b79b Compare July 16, 2024 09:57
@2eha0 2eha0 requested a review from a team as a code owner July 17, 2024 07:07
@2eha0 2eha0 force-pushed the feat/KM-299-router-playground branch 4 times, most recently from a9bd8c4 to 6760110 Compare July 18, 2024 06:54
Copy link
Contributor

@kaiarrowood kaiarrowood left a comment

Choose a reason for hiding this comment

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

Some nits and suggestions. Did not functional test

packages/core/expressions/docs/router-playground-modal.md Outdated Show resolved Hide resolved
packages/core/expressions/docs/router-playground-modal.md Outdated Show resolved Hide resolved
packages/core/expressions/docs/router-playground-modal.md Outdated Show resolved Hide resolved
packages/core/expressions/docs/router-playground-modal.md Outdated Show resolved Hide resolved
packages/core/expressions/src/locales/en.json Outdated Show resolved Hide resolved
packages/entities/entities-routes/docs/route-form.md Outdated Show resolved Hide resolved
@2eha0 2eha0 force-pushed the feat/KM-299-router-playground branch 2 times, most recently from c13b136 to 827a61a Compare July 31, 2024 09:48
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
packages/core/expressions/cypress.config.ts Outdated Show resolved Hide resolved
packages/core/expressions/package.json Show resolved Hide resolved
packages/entities/entities-routes/cypress.config.ts Outdated Show resolved Hide resolved
packages/entities/entities-routes/vite.config.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@ValeryG ValeryG left a comment

Choose a reason for hiding this comment

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

I think introducing the mix of packages that share cypress.config and another set op packages that requires it's of cypress config Is a mistake. This makes CI (already complicated) even more complicated.

This makes me as a developer trying to create new package even more confused - do I do it with my own config or do I use shared...

We need to find the way of doing it unified way. If it has to be cypress config for each package - let's do it for every package. Have shared config as a function and extend it for packages that are needed.

But let's keep CI and package structure universal.

@adamdehaven
Copy link
Member

adamdehaven commented Aug 2, 2024

I agree with @ValeryG here. We have an existing pattern in konnect-ui-apps - if we need to change, we should be more similar across repos.

@2eha0 I'd recommend moving the test-related changes out of this PR if you need the rest of the component feature work.

@2eha0
Copy link
Contributor Author

2eha0 commented Aug 5, 2024

@adamdehaven @ValeryG
Alright, It seems that the current test configuration method needs to be refactored. I'll create another PR to address this, For now, in this PR, I will revert the CI changes and skip some tests.

@2eha0 2eha0 force-pushed the feat/KM-299-router-playground branch 2 times, most recently from a4081f2 to ad08840 Compare August 5, 2024 05:57
@2eha0 2eha0 requested review from ValeryG and adamdehaven August 5, 2024 05:59
@2eha0 2eha0 force-pushed the feat/KM-299-router-playground branch from ad08840 to e351421 Compare August 5, 2024 06:11
@adamdehaven
Copy link
Member

@adamdehaven @ValeryG Alright, It seems that the current test configuration method needs to be refactored. I'll create another PR to address this, For now, in this PR, I will revert the CI changes and skip some tests.

Rather than refactoring the test configuration, it may be better to write up a summary of why the current setup does not meet your needs, and let the Core UI team evaluate any changes we can implement so that it works better for everyone, rather than integrating changes for a specific package.

@adamdehaven adamdehaven dismissed their stale review August 7, 2024 13:18

Comments addressed

ValeryG
ValeryG previously approved these changes Aug 7, 2024
Copy link
Member

@sumimakito sumimakito left a comment

Choose a reason for hiding this comment

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

Did functionality tests and LGTM 🚀

We have some known issues in the router playground:

  • HTTP headers in requests appeared as [object Object] in the exported JSON
  • Support HTTP headers with multiple values (should add context value per header value)
  • Better ID handling while importing/exporting requests
  • Missing VFG in the sandbox causing Vue warnings

As the main goal of this PR is to move the playground from kong-admin to here, we can address them as separate follow-ups to avoid blocking this one. I've created KM-391 to track them.

Please hold off merging this until we have green CI in adoption PRs.

@sumimakito
Copy link
Member

Tested functionality in Kong Manager and Konnect Gateway Manager. LGTM

@2eha0 2eha0 force-pushed the feat/KM-299-router-playground branch from e351421 to 019debf Compare August 8, 2024 08:57
@sumimakito sumimakito enabled auto-merge (squash) August 8, 2024 09:33
@sumimakito sumimakito merged commit b60b9bb into main Aug 8, 2024
34 checks passed
@sumimakito sumimakito deleted the feat/KM-299-router-playground branch August 8, 2024 09:40
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.

8 participants