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

validate command #200

Merged
merged 27 commits into from Oct 12, 2020
Merged

validate command #200

merged 27 commits into from Oct 12, 2020

Conversation

uwedeportivo
Copy link
Contributor

@uwedeportivo uwedeportivo commented May 8, 2020

Adds a new validate command to src-cli. This command validates a Sourcegraph instance by executing a script given as an argument. The intention is to provide a way for admins of Sourcegraph instances to verify that their new install or their upgrade went well and the instance is functioning.

The validation steps are specified in a json structure. The validation follows this outline:

  • create the first admin user
  • use their credentials to create an external service
  • wait for repo to clone
  • execute a search and check the count on the result
  • delete the external service

The spec is something like this (if some of the spec structs are not declared, then the corresponding step is skipped)

{
  "firstAdmin": {
    "email": "e2e@sourcegrah.com",
    "username": "e2e-test-user",
    "password": "123123123-e2e-test"
  },
  "externalService": {
    "config": {
      "url": "https://github.com",
      "token": "{{ .github_token }}",
      "orgs": [],
      "repos": [
        "sourcegraph-testing/zap"
      ]
    },
    "kind": "GITHUB",
    "displayName": "footest",
    "deleteWhenDone": true
  },
  "waitRepoCloned": {
    "repo": "github.com/sourcegraph-testing/zap",
    "maxTries": 5,
    "sleepBetweenTriesSeconds": 2
  },
  "searchQuery": "repo:^github.com/sourcegraph-testing/zap$ SugaredLogger count:99999"
}

documentation for the command is here sourcegraph/sourcegraph#12001

@dadlerj
Copy link
Member

dadlerj commented May 8, 2020

For the customer use case, it's nice to have a flexible tool for this, but I worry that it's too complex/time-intensive to expect them to write their own tests. Would we ship this with a built-in test suite that we recommend?

@uwedeportivo
Copy link
Contributor Author

For the customer use case, it's nice to have a flexible tool for this, but I worry that it's too complex/time-intensive to expect them to write their own tests. Would we ship this with a built-in test suite that we recommend?

We will provide examples that customers can edit and insert their own repo, token, external services details. Does that address your concerns ? Customers don't need to use this if they don't feel a need for it. But I think the simple syntax and the provided docs and examples will make this not be an issue imho.

@unknwon
Copy link
Member

unknwon commented May 9, 2020

This is great stuff! But I have the similar concern as @dadlerj that writing this kind of tests actually requires the customer/admin being proficient of how Sourcegraph actually works internally?

@uwedeportivo
Copy link
Contributor Author

@unknwon i'm not sure i understand your comment: why would admins need to know the internals. the scripting only exposes operations that one would do in the UI or in the graphql API console.

@dadlerj
Copy link
Member

dadlerj commented May 11, 2020

My POV:

As long as (eventually) no customers need to learn this command language, I'm happy. If the user experience is src validate <default-smoke-test-script> that they can run after upgrading, then I'd be thrilled! It's nice if really active/enterprising customers can then author their own to expand it, but they shouldn't have to author their own to get a basic test done.

@uwedeportivo
Copy link
Contributor Author

@dadlerj the problem is that they do have to provide which repos they want to add and which search queries they want to execute (etc). this varies from customer to customer. we could have them all use one of our sourcegraph-testing repos, but some of the customers don't even want or can't access github.com. they also need to provide their own github token even if they do have access to github.com.

that's why i thought this is a good compromise. it's simple, restricted enough but a good place to pass this information into the validation. and flexible enough that customers can use their own setup and tokens and decide which verifications and steps are important to them.

we could narrow it down even further with them providing a repo name, the github token etc in a config file. but explaining and documenting these pieces of information gets you almost to this scripting.

cmd/src/main.go Outdated Show resolved Hide resolved
@beyang
Copy link
Member

beyang commented May 11, 2020

I support the idea of enabling customers to write their own validation scripts to exercise Sourcegraph after updating and think this is a great start. Here are my concerns:

  • Is skylark the right language to commit to? I don't know anything about it beyond that it's Python-like and it's the language Google uses for Blaze/Bazel. It does not seem widely used outside of that.
  • Should we first test this out more thoroughly ourselves before asking customers to invest time in writing their own validation scripts? Once customers start using it, we are on the hook for supporting it, which means maintaining it in the long-run or providing excellent assistance for migrating customers off of it if we abandon it in the future.

If the goal here is just to rapidly prototype this as a proof-of-concept and use it ourselves, then I would be okay approving this under the condition that it is very very clear that it is experimental and unsupported, and it could disappear or change in a breaking fashion at any time.

I would also like to see more examples of tests written using this subcommand that we use ourselves in our testing/release pipeline. That would help me determine if this is worth shipping to customers.

@uwedeportivo
Copy link
Contributor Author

@beyang i agree with all your points. this is very much experimental. i needed a vehicle for deployment and chose src-cli. customers won't hear about it until we're sure we want to go down this road. if they discover it by accident then it's clearly marked as experimental. we won't advertise it.

i'm also not sure about the scripting language choice. i wanted something simple with a familiar syntax. we can plug in a different choice without too much effort.

i will use deploy-sourcegraph as the use case to kick the tires on it and test the approach.

Co-authored-by: Beyang Liu <beyang@sourcegraph.com>
@uwedeportivo
Copy link
Contributor Author

sounds good. i'll call it out more and try to re-use what's there (there isn't much for adding and removing)

Copy link
Member

@daxmc99 daxmc99 left a comment

Choose a reason for hiding this comment

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

cmd/src/validate.go Outdated Show resolved Hide resolved
cmd/src/validate.go Outdated Show resolved Hide resolved

var (
contextFlag = flagSet.String("context", "", `Comma-separated list of key=value pairs to add to the script execution context`)
docFlag = flagSet.Bool("doc", false, `Show documentation`)
Copy link
Member

Choose a reason for hiding this comment

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

Remove and just have it print the URL as part of the help output instead? I don't understand why a -doc flag is useful when -h exists for this purpose :)

}

func (vd *validator) printDocumentation() {
fmt.Println("Please visit https://docs.sourcegraph.com/admin/validation for documentation of the validate command.")
Copy link
Member

Choose a reason for hiding this comment

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

Please add docs for this command to the handbook under distribution (or the actual docs if we want it public). Specifically the config format being documented would be nice.

Copy link
Member

@slimsag slimsag left a comment

Choose a reason for hiding this comment

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

Awesome, looks really good to me!

@LawnGnome LawnGnome changed the base branch from master to main August 21, 2020 21:04
@christinaforney christinaforney removed their request for review August 28, 2020 16:15
@beyang beyang removed their request for review September 1, 2020 19:41
slimsag added a commit to sourcegraph/sourcegraph that referenced this pull request Oct 9, 2020
This updates the regression test docs to clarify how to run these tests -
these docs were really outdated / broken, I had to do a fair amount of digging.

I added a 1password note with the `.envrc` file I got from Uwe, because you
need to set like ~15 env vars in order to run these tests.

I also removed the comments about wanting customers in the future to run this
test suite - I don't think we want that anymore and that experience would not
be good as running this test suite requires a working Sourcegraph dev env. The
`src validate` command Uwe added to src-cli seems a lot better from a customer
POV: sourcegraph/src-cli#200
@mrnugget
Copy link
Contributor

mrnugget commented Oct 9, 2020

What's the status here? I get pinged by the GitHub bot in Slack about this PR :)

uwedeportivo and others added 5 commits October 12, 2020 07:20
Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
@uwedeportivo uwedeportivo merged commit 83426b6 into main Oct 12, 2020
OLD - Distribution - use "Distribution 🚢" automation moved this from Medium priority (ordered) to Done Oct 12, 2020
@uwedeportivo uwedeportivo deleted the validate_instance_cmd branch October 12, 2020 19:13
@uwedeportivo
Copy link
Contributor Author

@mrnugget it landed :-)

@mrnugget
Copy link
Contributor

@uwedeportivo Can you add a CHANGELOG entry?

slimsag added a commit to sourcegraph/sourcegraph that referenced this pull request Oct 13, 2020
This updates the regression test docs to clarify how to run these tests -
these docs were really outdated / broken, I had to do a fair amount of digging.

I added a 1password note with the `.envrc` file I got from Uwe, because you
need to set like ~15 env vars in order to run these tests.

I also removed the comments about wanting customers in the future to run this
test suite - I don't think we want that anymore and that experience would not
be good as running this test suite requires a working Sourcegraph dev env. The
`src validate` command Uwe added to src-cli seems a lot better from a customer
POV: sourcegraph/src-cli#200
slimsag added a commit to sourcegraph/sourcegraph that referenced this pull request Oct 13, 2020
This updates the regression test docs to clarify how to run these tests -
these docs were really outdated / broken, I had to do a fair amount of digging.

I added a 1password note with the `.envrc` file I got from Uwe, because you
need to set like ~15 env vars in order to run these tests.

I also removed the comments about wanting customers in the future to run this
test suite - I don't think we want that anymore and that experience would not
be good as running this test suite requires a working Sourcegraph dev env. The
`src validate` command Uwe added to src-cli seems a lot better from a customer
POV: sourcegraph/src-cli#200
scjohns pushed a commit that referenced this pull request Apr 24, 2023
* validate command

* fleshed out a bit more

* more fns

* fns tweaks

* fns fixes

* ability to create first admin and use session credentials instead of access token

* fixes

* fix extra client

* take over starlight

* point to sourcegraph/starlight

* Update cmd/src/main.go

Co-authored-by: Beyang Liu <beyang@sourcegraph.com>

* hide validate since it is super experimental

* fixes

* add validate cmd dependencies

* remove starlark

* clean up go mod

* specification as YAML and pointer to docs

* Update cmd/src/validate.go

Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>

* Update cmd/src/validate.go

Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>

* catch up

* list cloned repos graphql

Co-authored-by: Beyang Liu <beyang@sourcegraph.com>
Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants