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

add healthcheck in testsuite for armada server #1120

Merged

Conversation

dejanzele
Copy link
Member

No description provided.

testsuite/file_test.go Outdated Show resolved Hide resolved
pkg/client/connection.go Show resolved Hide resolved
@@ -17,6 +22,7 @@ import (

type ApiConnectionDetails struct {
ArmadaUrl string
ArmadaRestUrl string
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to just construct the ArmadaRestUrl from ArmadaUrl?

I don't love a new config value that requires our client users to put both a RestURL and a grpc URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

No. They may be completely different.

Copy link
Contributor

Choose a reason for hiding this comment

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

So how does someone understand what to put for ArmadaRestUrl?

This is a config that our users of armadactl and other client libs use so I don't understand how they would know that the RestURL that grpc is generating could be very different from the connection details they would use for the internal grpc server.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kannon92 grpc and rest could quite possibly be on completely different (sub)domains, where the REST server exposes "safe" endpoints and could be made public, and the gRPC server exposes "unsafe/admin" endpoints, and could be made private.

cmd/testsuite/cmd/test.go Outdated Show resolved Hide resolved
cmd/testsuite/cmd/test.go Outdated Show resolved Hide resolved
testSuites.AddSuite(*testSuite)
if junitPath != "" {
if err := createJUnitReport(junitPath, testSuites); err != nil {
return errors.Wrap(err, "error creating junit report")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return errors.Wrap(err, "error creating junit report")
return errors.WithMessage(err, "error creating junit report")

cmd/testsuite/cmd/test.go Outdated Show resolved Hide resolved
cmd/testsuite/cmd/test.go Outdated Show resolved Hide resolved
pkg/client/connection.go Outdated Show resolved Hide resolved
pkg/client/connection.go Outdated Show resolved Hide resolved
pkg/client/connection.go Outdated Show resolved Hide resolved
@severinson severinson enabled auto-merge (squash) June 27, 2022 16:42
@severinson severinson merged commit 23b4268 into armadaproject:master Jun 27, 2022
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.

None yet

3 participants