feat: reverse proxy #336
feat: reverse proxy #336
Conversation
|
Could you please provide the commands of how you testet it locally? |
|
I started reviewing and to understand it better I have two questions:
|
yes it can be run via this command: go test -v ./e2e/basic/ --run TestReverseProxy -timeout 60mit creates a dummy instance with bittwister enabled and tries to call some APIs of bittwister to validate the proxy function. |
Since the issue doesn't have a lot of context, to help with alignment on async reviews, can we add some more detail to either the issue or the PR description? Things I'd like to know are:
|
e2e/basic/reverse_proxy_test.go
Outdated
| // Check if the BitTwister service is set | ||
| out, err := main.BitTwister.Client().AllServicesStatus() | ||
| require.NoError(t, err, "Error getting all services status") | ||
| assert.NotEmpty(t, out, "No services found") |
There was a problem hiding this comment.
If possible it would be good to check for some explicit state other than not empty as this would not protect us from a bug in the code that returns an invalid non empty response.
There was a problem hiding this comment.
we could also use an nginx server instead
There was a problem hiding this comment.
delicate point, updated
There was a problem hiding this comment.
There was a problem hiding this comment.
instead of just checking if it is empty or not, now we check for existence of a particular field which can yield only if the request goes through successfully.
pkg/k8s/errors.go
Outdated
There was a problem hiding this comment.
[follow up] we should enable golangci-lint for this repo and enable the export comment linter.
Why?
Exports should have comments so when other packages import them, it is abundantly clear what they are importing.
The additional benefit of having comments is that it forces line separation for things like this. So when 1 new error is added, we don't have an unnecessarily long diff that is unhelpful in understanding the actual change.
Copilot can add comments to all of these, so it isn't a big lift to add comments and new lines here in a separate PR.
Could be done now and merge to clean up the diff here.
There was a problem hiding this comment.
yeah using a linter is a great idea in ci.
About the comments on Exported funcs, I blieve not every function needs it. It is attempted to have clear names for funcs and their arguments so it will be intuitive for users to use it. Otherwise yes we need comments to clarify stuff or even show an example to the users.
There was a problem hiding this comment.
We can have the discussion around which linters to enable, but we should make a follow up to track getting the linters on the code base.
There was a problem hiding this comment.
an issue is created for it
|
In the prototype for big block tests, I added the following function to func (i *Instance) AddHost(port int) (err error, host string) {
prefix := fmt.Sprintf("%s-%d", i.k8sName, port)
if err := traefikClient.AddHost(context.TODO(), i.k8sName, prefix, port); err != nil {
return ErrAddingHost.WithParams(i.k8sName).Wrap(err), ""
}
host, err = traefikClient.URL(context.TODO(), prefix)
if err != nil {
return ErrGettingHost.WithParams(i.k8sName).Wrap(err), ""
}
return nil, host
}It would be great to have a function with that functionality. Feel free to rename and modify the function at will. |
Co-authored-by: Jose Ramon Mañes <32740567+jrmanes@users.noreply.github.com>
Makefile
Outdated
There was a problem hiding this comment.
Can be a follow up but imo it would be better to DRY up this commands and just use the pkg and run variables.
ie:
pkgs := $(shell go list ./...)
run := .
count := 1
test:
KNUU_TIMEOUT=120m go test -v $(pkgs) -run $(run) -count=$(count) -timeout 120m
.PHONY: testthen you can just have the one makefile command and easily target whatever by setting the pkg, run, or count flags.
Targeting a directory
make test pkgs=./e2e/basic
Targeting a Test in a directory
make test pkgs=./e2e/basic run=TestJustThisTest
Run a test in a loop to debug
make test pkgs=./e2e/basic run=TestJustThisTest10Times count=10
This model has more flexibility and DRY's up the Makefile significantly.
There was a problem hiding this comment.
that's a good idea. specially when we add unit tests this make file can grow really fast. This way we can have a cleaner code. Let's address it in a separate PR.
There was a problem hiding this comment.
I don't think my previous review was fully resolved.
Please don't resolve a conversations unless the action was fully implemented with no deviation.
Responding to a comment and then dismissing the review by resolving the discussion makes it harder for re-reviews. As it isn't clear if comments were fully addressed so then reviewers have to go through and open up all the discussions to check. Or the same comments will just be left again.
Signed-off-by: Jose Ramon Mañes <jose@celestia.org>
This is a draft version as there is an issue which needs help from other team members.
To run it, use the following command
Closes #315