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(gui): upgrade gui to latest #4338

Merged
merged 1 commit into from
May 24, 2022
Merged

feat(gui): upgrade gui to latest #4338

merged 1 commit into from
May 24, 2022

Conversation

lahabana
Copy link
Contributor

Signed-off-by: Charly Molter charly.molter@konghq.com

@lahabana lahabana requested a review from a team as a code owner May 24, 2022 11:06
Copy link
Contributor

@slonka slonka left a comment

Choose a reason for hiding this comment

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

Not really anything that I could review here, is there a way to verify that this is actually the gui / which version it is? Just a general question, why do we have GUI in a separate project?

@lahabana lahabana changed the title feat(gui): Upgrade gui to latest feat(gui): upgrade gui to latest May 24, 2022
@lahabana
Copy link
Contributor Author

Great questions!

Not really anything that I could review here, is there a way to verify that this is actually the gui / which version it is?
There's an issue somewhere about just having the whole thing work as "commit" reference that's pulled on demand. So updating the GUI would be updating a git ref which would be much easier to review (also I'm not a fan of checking in generated code).

why do we have GUI in a separate project?
I don't maybe because technically it's 2 different teams. Maybe it's simpler in the enterprise case (the enterprise GUI is a fork of the original GUI). @jakubdyszkiewicz or @bartsmykla would know more maybe.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2022

Codecov Report

Merging #4338 (5d451d7) into master (56280f6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4338   +/-   ##
=======================================
  Coverage   55.38%   55.38%           
=======================================
  Files         941      941           
  Lines       57125    57125           
=======================================
  Hits        31641    31641           
- Misses      22974    22979    +5     
+ Partials     2510     2505    -5     
Impacted Files Coverage Δ
pkg/plugins/common/postgres/listener.go 53.84% <0.00%> (-15.39%) ⬇️
pkg/plugins/resources/postgres/events/listener.go 16.66% <0.00%> (-14.59%) ⬇️
pkg/core/tokens/default_signing_key.go 66.66% <0.00%> (-5.56%) ⬇️
pkg/plugins/runtime/gateway/route/sorter.go 66.66% <0.00%> (-5.13%) ⬇️
api/observability/v1/mads.pb.go 35.56% <0.00%> (+1.03%) ⬆️
pkg/mads/v1/client/client.go 43.75% <0.00%> (+2.50%) ⬆️
pkg/mads/server/server.go 85.45% <0.00%> (+2.72%) ⬆️
pkg/plugins/resources/postgres/migrate.go 61.90% <0.00%> (+6.34%) ⬆️
pkg/plugins/common/postgres/connection.go 66.66% <0.00%> (+16.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56280f6...5d451d7. Read the comment docs.

@bartsmykla
Copy link
Contributor

I would ask opposite question - why we have GUI inside main repo ;-) I would say it should live in separate as it's now, and instead of manually updating artifacts to the gui directory, it should be included during the release process. I would even start a discussion if it could be fully removed from the main repo/project and be "installed" when deploying to the clusters?

I think the decision to do it as it's now was dictated as it's just easier to have it as it's now

Signed-off-by: Charly Molter <charly.molter@konghq.com>
@lahabana
Copy link
Contributor Author

The issue is #3863

I would even start a discussion if it could be fully removed from the main repo/project and be "installed" when deploying to the clusters?

That would open a bunch of issues in airlocked envs no?

@lahabana lahabana enabled auto-merge (squash) May 24, 2022 11:32
@slonka
Copy link
Contributor

slonka commented May 24, 2022

why we have GUI inside main repo?

From the top of my head:

  • when changing anything related to both Kuma and GUI I don't have to remember to check the other repo and have separate PRs for the same thing
  • we can have more inclusive integration/e2e tests
  • it's easier to type check API compatibility, we could import the proto types from Kuma to the GUI types in typescript so we are sure on the type level we're not making any breaking changes
  • we're not shipping it separately, I don't think we want another problem of GUI version X is compatible with Kuma Y, Z...
  • we wouldn't even have to bump it, it's right there included together

@lahabana lahabana merged commit 05c7981 into kumahq:master May 24, 2022
@slonka
Copy link
Contributor

slonka commented May 27, 2022

@bartsmykla @lahabana any thoughts on ☝️ #4338 (comment)

@lahabana
Copy link
Contributor Author

it's easier to type check API compatibility, we could import the proto types from Kuma to the GUI types in typescript so we are sure on the type level we're not making any breaking changes

In practice the real fix to this is correct openAPI spec so that the JS client can be auto-generated.

I guess another issue would be for us to work with the enterprise version of the GUI. We could always work-around this some other way though...

Anyway I've referenced the issue where this will be dealt with. I don't think this is something we need to address right now so just referencing this for future reference.
The day we restart serious work on the GUI we should have this discussion again to make something quicker and more sustainable for sure!

@slonka
Copy link
Contributor

slonka commented May 31, 2022

Agreed, thanks!

@lahabana lahabana deleted the guiUp branch March 29, 2024 12:46
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