Separate UI #1469
Merged
Separate UI #1469
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ozamosi
reviewed
Feb 17, 2022
gitops.dockerfile
Outdated
| COPY Makefile /app/ | ||
| COPY tools /app/tools | ||
| WORKDIR /app | ||
| RUN make dependencies |
There was a problem hiding this comment.
Do we really need to add make dependencies here? This is the one that starts using sudo inside your container.
SamLR
force-pushed
the
separate-ui
branch
3 times, most recently
from
92e81ee
to
69e7db2
Compare
ozamosi
approved these changes
Feb 21, 2022
added 8 commits
February 21, 2022 12:03
This will be moved to a standalone binary
We want to remove the `ui run` command and create a separate binary which will run the gitops ui. This (very brutishly) copies the existing ui run code over the gitops-server code to be used as a separate binary. The gitops-server is being over-written as it was apparently created as a convenience for developing the UI. As part of this work that should become obsolete as a pattern and be replaced by the actual UI binary
To separate out the UI components from the cli tool we need to update the makefile to generate the dist/ui files in the `gitops-server` rather than `gitops` directories. This additionally changes to using a pattern-rule target to generate the go binaries so that the same command can be used shared.
The `ui` command will display information on how to access the gitops server and/or establish port-forwarding for it. This placeholder preserves this extension point
We will need a docker image of the server component in order to deploy it to k8s. It's less clear that a docker image of the cli component is needed but is likely useful in some circumstances (e.g. local testing without installation). This creates two docker files, one for the cli and another for the server.
Main change is the removal of browser and isatty becoming indirect.
This may be removed soon but until then it's needed for the cli tool to run
|
@JamWils @katya-makarevich @dlopro not sure who else I should ask to review this |
Closed
luizbafilho
approved these changes
Feb 23, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Remove the
ui runcommand and replace with a separate server binaryWhy?
How did you test it?
The binaries build and successfully host the UI/operate on the CLI.
Release notes
This is a breaking change that removes the
ui runcommand (theuicommand has been replaced with a placeholder that can be extended to help access the ui).Main questions/decisions to focus on for review
I've mostly left the Go code intact as I'm not so confident there and it feels like more of that will be wanted by the observability folks.
This means the focus of a lot of this has been around dockerfiles and the makefile. The later especially as the next bit of work I have is to tidy it up further. To that end I have some questions about how comfortable folks are with various usages of makefiles:
cmd/gitops/ui/run/dist/,cmd/gitops/ui/run/dist/index.htmlandcmd/gitops/ui/run/dist/index.jstargets?bin/%: cmd/%/main.go) to reduce repetition or are they more overhead than is worth it?