Fix context UI add formatting refactor #57

chanwit · 2021-09-09T09:27:28Z

This PR

fixed K8s context listing bug
added code formatting for TypeScript and Go, including ESLit and Go Vet
did some refactoring
updated CRDs to those of Flux v0.17.0

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

squaremo · 2021-09-09T09:33:17Z

Thanks @chanwit! Would you please split this into PRs that accomplish one thing (or near enough) -- maybe

put formatting and linting in the build
fix bug and refactoring (if the refactoring pertains to the code you were fixing)
update the CRDs (unless that is part of fixing, or refactoring)

jpellizzari

@chanwit Can you describe the bug this is trying to fix?

jpellizzari · 2021-09-09T13:19:15Z

.eslintrc

@@ -4,7 +4,8 @@
        "plugin:import/errors",
        "plugin:import/warnings",
        "plugin:@typescript-eslint/recommended",
-        "plugin:import/typescript"
+        "plugin:import/typescript",
+        "plugin:prettier/recommended" // eslint shows prettier formatting warnings. Must be always the last configuration.


This may cause conflicts with typescript. Having two formmatters running will cause them to over-write each others' changes.

I'm not very good at this kind of configuration.

Did you mean that we could just use eslint --fix and the source would get formatted without using prettier? If so, I'm happy to remove it.

jpellizzari · 2021-09-09T13:23:20Z

ui/components/TopNav.tsx

  const { navigate, currentPage } = useNavigation();

+  const contextSelectChanged = (ev) => {


Why was this moved?

Embedding event handlers into the view is too hard to read.

jpellizzari · 2021-09-09T13:24:20Z

ui/lib/rpc/twirp.ts

@@ -1,38 +1,46 @@
-
 export interface TwirpErrorJSON {


This is a generated file. When make proto is run, this formatting will be over-written.

jpellizzari · 2021-09-09T13:31:07Z

Makefile


-all: test build
+all: dist/index.html fmt vet test build


We probably shouldn't do "write" operations (formatting) on the make all commands. I wouldn't expect a make all command to leave us with a (potentially) modified git state.

We did that in Flux CLI. Go Fmt is part of the make file. Should we do this @stefanprodan?

I agree with Jordan here; I think it's OK to have phony targets that rewrite files, but the main build shouldn't create (more) diffs. If it does, it encourages commits which contain code changes then some random reformatting elsewhere.

This can make it difficult to reconcile with builds in which you commit generated files. For that, you can make people explicitly regenerate the file, and have a guard in the tests (and CI) to make sure the committed, generated file is correct.

(Also see go build -mod=readonly.)

(You can call me a hypocrite, since all the flux controllers inherit a Makefile structure which does fmt in the build, from kubebuilder. I don't notice because I've told emacs to run go fmt when I save a file.)

jpellizzari · 2021-09-09T13:34:45Z

main.go

-	for _, c := range rules.Contexts {
-		contexts = append(contexts, c.Cluster)
+	for contextName := range rules.Contexts {
+		contexts = append(contexts, contextName)


We were accessing c.Cluster before. Do we no longer need to do that?

jpellizzari · 2021-09-09T13:36:39Z

pkg/rpc/clusters/clusters.twirp.go

@@ -1,39 +1,32 @@
-// Code generated by protoc-gen-twirp v7.1.1, DO NOT EDIT.


I would not have expected this file to change. We probably need to use buf instead to avoid this happening in the future. Not something that needs to be taken care of by this PR, but we should probably revert this change for now.

jpellizzari · 2021-09-09T13:41:02Z

install/flux-webui.sh

@@ -0,0 +1,223 @@
+#!/usr/bin/env bash


I think we should have users manage their own installation. In addition, anyone who downloads the binary won't have this script, and if they have cloned the repo, they are probably intending to build from source.

There's a plan to have a brew/tap, yes.

jpellizzari · 2021-09-09T13:47:19Z

Makefile


 dist/index.html:
 	npm run build

+fmt:
+	go fmt ./...
+	npx prettier --write 'ui/**/*.ts(x)?'


I think one formatter is the way to go here. Two formatters may end up conflicting with one another.

chanwit · 2021-09-09T14:15:33Z

@jpellizzari I am in process of splitting this PR into smaller ones as @squaremo pointed out.

For the bug fixing part, there's a bug when we obtaining the list of contexts. Please see its structure and you'll see that we used the wrong field, c.Cluster.
c.Cluster is the cluster name, but not the context name.
The context name appears to be the key portion of rules.Contexts map.
I found this bug accidentally because a cluster name and a context on my machine are different.

jpellizzari · 2021-09-09T14:17:52Z

@jpellizzari I am in process of splitting this PR into smaller ones as @squaremo pointed out.

For the bug fixing part, there's a bug when we obtaining the list of contexts. Please see its structure and you'll see that we used the wrong field, c.Cluster.
c.Cluster is the cluster name, but not the context name.
The context name appears to be the key portion of rules.Contexts map.
I found this bug accidentally because a cluster name and a context on my machine are different.

Excellent! Nice find. I will await those PRs; feel free to add me as a reviewer when they are ready. Thanks for the contribution.

squaremo · 2021-09-09T14:18:45Z

we used the wrong field, c.Cluster.
c.Cluster is the cluster name, but not the context name.
The context name appears to be the key portion of rules.Contexts map.

Please be sure to put this explanation in the commit message :-)

chanwit added 11 commits September 9, 2021 16:22

update Flux CRDs

12d02f2

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

add makefile targets for code formatting and check

3864290

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

add code formatter for TypeScript

b5d5f62

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

fix context listing bug

e1bec12

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

fix label of context combobox and refactoring

858defe

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

update go module dependencies

2a03952

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

rename labels to reflect Flux resources

c19baec

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

regenerated from protobuf

e28a229

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

rename binary in readme

590ea3c

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

add one-line installation script

de62f48

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

update gitignore

e8301cb

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

chanwit mentioned this pull request Sep 9, 2021

Update flux crds #58

Open

jpellizzari reviewed Sep 9, 2021

View reviewed changes

adberger mentioned this pull request Nov 15, 2021

Relocate top navigation event handling outside of the view #63

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix context UI add formatting refactor #57

Fix context UI add formatting refactor #57

chanwit commented Sep 9, 2021 •

edited

Loading

squaremo commented Sep 9, 2021

jpellizzari left a comment

jpellizzari Sep 9, 2021

chanwit Sep 9, 2021

jpellizzari Sep 9, 2021

chanwit Sep 9, 2021

jpellizzari Sep 9, 2021

jpellizzari Sep 9, 2021

chanwit Sep 9, 2021

squaremo Sep 9, 2021

squaremo Sep 9, 2021

jpellizzari Sep 9, 2021

jpellizzari Sep 9, 2021 •

edited

Loading

jpellizzari Sep 9, 2021

chanwit Sep 9, 2021

jpellizzari Sep 9, 2021

chanwit commented Sep 9, 2021 •

edited

Loading

jpellizzari commented Sep 9, 2021

squaremo commented Sep 9, 2021

		const { navigate, currentPage } = useNavigation();

		const contextSelectChanged = (ev) => {

		@@ -1,39 +1,32 @@
		// Code generated by protoc-gen-twirp v7.1.1, DO NOT EDIT.

Fix context UI add formatting refactor #57

Are you sure you want to change the base?

Fix context UI add formatting refactor #57

Conversation

chanwit commented Sep 9, 2021 • edited Loading

squaremo commented Sep 9, 2021

jpellizzari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpellizzari Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanwit commented Sep 9, 2021 • edited Loading

jpellizzari commented Sep 9, 2021

squaremo commented Sep 9, 2021

chanwit commented Sep 9, 2021 •

edited

Loading

jpellizzari Sep 9, 2021 •

edited

Loading

chanwit commented Sep 9, 2021 •

edited

Loading