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

chore(test): add layout for dgraphtest package #8707

Merged
merged 7 commits into from
Mar 7, 2023
Merged

Conversation

mangalaman93
Copy link
Contributor

@mangalaman93 mangalaman93 commented Feb 25, 2023

This package will provide functions to setup cluster for testing and benchmarking going forward

@github-actions github-actions bot added the go Pull requests that update Go code label Feb 25, 2023
@mangalaman93 mangalaman93 force-pushed the aman/upgrade branch 7 times, most recently from aa6d38a to 7de7bf6 Compare February 25, 2023 20:54
@mangalaman93 mangalaman93 marked this pull request as ready for review February 27, 2023 05:12
@coveralls
Copy link

coveralls commented Feb 27, 2023

Coverage Status

Coverage: 67.143% (+0.009%) from 67.134% when pulling c8c22a2 on aman/upgrade into 1a182e6 on main.

Copy link
Contributor Author

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

one comment, good to go otherwise

dgraphtest/config.go Outdated Show resolved Hide resolved
@mangalaman93 mangalaman93 marked this pull request as ready for review March 2, 2023 17:40
@@ -29,8 +29,6 @@ RUN apt-get update && apt-get install -y --no-install-recommends \

ADD linux /usr/local/bin

EXPOSE 8080
EXPOSE 9080
Copy link
Contributor

@skrdgraph skrdgraph Mar 3, 2023

Choose a reason for hiding this comment

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

This is the same Dockerfile that is being used for build, test & release. This could break things for folks who run the image without port-mapping. https://stackoverflow.com/a/22150099 #2 is important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone is running a Dgraph Cluster, I am wondering how only exposing alpha ports will make things work. We are not exposing zero ports here.

@skrdgraph
Copy link
Contributor

@mangalaman93 this may be redundant, I understand the purpose of this. But have you looked at https://github.com/dgraph-io/dgraph/blob/main/compose/compose.go ...

The old team (from what I can tell) was planning to transition t.go dgraph-cluster CRUDs to compose.go. This wrapper tool is actually quite handy. If it can be repurposed it will serve your purpose IMO.

@mangalaman93
Copy link
Contributor Author

The compose tool gives you a very static cluster. For our testing going forward, we would like a more dynamic control on the cluster. Think about upgrades for example, we want to setup a cluster with a dgraph version for which we don't even have a docker image. This could be easily done if we have the binary and we can mount that binary inside the container. We can also do the same when we want to upgrade the cluster and so on. This is just the base of the work, we are going to build more interesting tools on top of this work. I am also thinking that using the same library, we could setup cluster on AWS, ECS if needed and even a cluster on Dgraph cloud. The interface to the test would be consistent so that the same test could run in different environment. Maybe, we could integrate the compose tool as one of the ways of setting up the cluster in the library.

dgraphtest/cluster.go Outdated Show resolved Hide resolved
dgraphtest/cluster.go Outdated Show resolved Hide resolved
dgraphtest/cluster.go Outdated Show resolved Hide resolved
dgraphtest/cluster.go Outdated Show resolved Hide resolved
dgraphtest/dgraph.go Outdated Show resolved Hide resolved
dgraphtest/dgraph.go Show resolved Hide resolved
dgraphtest/dgraph.go Show resolved Hide resolved
dgraphtest/config.go Outdated Show resolved Hide resolved
dgraphtest/dgraph.go Show resolved Hide resolved
@@ -29,8 +29,6 @@ RUN apt-get update && apt-get install -y --no-install-recommends \

ADD linux /usr/local/bin

EXPOSE 8080
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it only works for alpha, it doesn't work for Zero

return resp.ID, nil
}

func (c *Cluster) healthCheck() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is checking for health the same as checking if license is applied? Today we got an issue where we were checking for health, but still license failed. Can we add a check for is license applied? Okay to be done in a separate diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you elaborate?

dgraphtest/cluster.go Outdated Show resolved Hide resolved
dgraphtest/image.go Show resolved Hide resolved
dgraphtest/config.go Show resolved Hide resolved
dgraphtest/config.go Show resolved Hide resolved
contrib/Dockerfile Outdated Show resolved Hide resolved
@mangalaman93 mangalaman93 merged commit e5d8d44 into main Mar 7, 2023
@mangalaman93 mangalaman93 deleted the aman/upgrade branch March 7, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code
Development

Successfully merging this pull request may close these issues.

6 participants