Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Conversation

@BolajiOlajide
Copy link
Contributor

git-subtree-dir: src-cli
git-subtree-split: efd12d5

Part 1 #61081

This is the first step towards moving src-cli into the monorepo. I have a 4-part plan to achieve this:

  1. Move the src-cli repository and it's git history into the monorepo. Also ensure this is merged and not squashed so it's history is browsable.
  2. Split the repository, currently contained in src-cli into it's relevant subdirectories.
  3. Fix CI and other workflows needed for src-cli.
  4. Archive the src-cli repospitory.

Test plan

Just a move. Nothing to test yet. Will fix CI and others in a subsequent PR.

olafurpg and others added 30 commits May 30, 2022 13:39
* Make src-cli installable via npm.

Fixes #759. Previously, the recommended way to install src-cli on Linux
was to download a static binary via `curl`. This PR makes it possible to
install src-cli via `npm install -g @sourcegraph/src`. The new npm
distribution will be particuarly helpful for users of our new
scip-typescript and scip-python indexers that are also distributed via
npm.

* Make the npm release flow depend on goreleaser
)

The last `npm publish` step failed with an authorization error
https://github.com/sourcegraph/src-cli/runs/6707002077?check_suite_focus=true
```
npm ERR! need auth This command requires you to be logged in to https://registry.npmjs.org/
29
```

Our current hypothesis is that this change fixes the issue.
Co-authored-by: Renovate Bot <bot@renovateapp.com>
Previously, you had to manually open a PR to bump up the version in package.json.
Now, the version is automatically bumped during the release pipeline.
The last npm release job failed with "error Invalid version supplied."
because the inferred version was "refs/tags/3.40.6".
This includes a fixup for an import that has moved from
sourcegraph/sourcegraph/lib to sourcegraph/scip since the last time we
updated the dependency.
This was reported by a user, we store execution step results in the cache also when no diff was generated (rightfully), but then we should only apply the patch when there is actually anything to do.

Also, I initially couldn't reproduce this locally because we swallowed this error in volume mode. It would only happen in bind mode.
This fixes two things, that I found today:

It changes Stdout and Stderr on the StepResult struct to string, it is part of the marshalled cache result and *bytes.Buffer cannot be marshalled into JSON. This caused stdout/stderr to always be empty, thus breaking executions where ${{ previous_step.stdout }} is used.
It fixes a misconception of the PreviousStepResult. When running the actual execution, it will eventually become the result of the previous step, but from the perspective of that particular cache result, it has to be the result from THAT step execution. Otherwise caching would only work from step 2 on and skip 1 step still.
…ditional call to workspace.Diff (#777)

This is the src-cli portion of sourcegraph/sourcegraph#36758. It also optimizes the execution generally, because we run one less workspace.Diff.
After this PR, nothing requires us to still write or read execution results from the cache. Since this is a bigger code cleanup/refactor, I filed a separate ticket to address this and put it in sustaining. This PR mostly focuses on getting the Diff call out of the execution and stops to write the unnecessary cache result to the JSON logs so we send less log data.
Docker Desktop seems to be prone to occasionally entering states where
it is nominally available (in the sense that it listens for
connections), but can't actually do anything useful. Memory exhaustion
events from previous `src batch` runs tend to be the culprit here.

This seems to be most easily detectable by looking at the behaviour of
`docker image inspect`. This command doesn't have to hit the network,
nor does it perform any real work, so it should always be "quick". If
it's not, that implies that Docker is not in a happy place.

This commit adds a five second timeout to `docker image inspect`
invocations, along with a secret environment variable to adjust this
behaviour. If `docker` times out, then a (hopefully) helpful error
message is generated to hint towards the things that the user should
investigate next (basically, "is Docker really working?").

The only real concern I have with this is that I've basically picked
five seconds out of thin air: practically, Docker for Mac seems to be
able to do this in tens of milliseconds, but Sourcegraph has bought me a
rather nice laptop. I have trouble imagining a scenario where multiple
second delays are common where there aren't other serious issues, but
the world is a large, weird place.
We parse lots of the things that git outputs, so having stderr randonly interfere with the output breaks that parsing. Since we can still get stderr from the exiterror, I don't think this is a regression at all in debuggability. This was found on k8s where parsing the patch would fail on a particular repo. Turns out git logged a warning.
This PR cleans up the src side of things for SSBC:
- Ensures to have a separate cache directory for each src running, so they don't collide while writing to them
- Ensures to have a tmp directory set within the executor workspace so it can be retained for inspection
- Removes useless API call to Sourcegraph to retrieve it's version
- Supports native repos cloned into the workspace
- Ensures no env var is used for cache key generation, those are not set on the server and it would cause cache key mismatches
- And most importantly, it doesn't require an access token anymore, so it's much safer, especially in less-isolated executors!

Closes https://github.com/sourcegraph/sourcegraph/issues/37079
Closes https://github.com/sourcegraph/sourcegraph/issues/36597
Closes https://github.com/sourcegraph/sourcegraph/issues/36187
* upsert batch change when running `src batch remote

* use batchChangeName for execution url

* update changelog

* add createBatchSpecFromRaw query

* Update CHANGELOG.md

Co-authored-by: Adam Harvey <adam@adamharvey.name>

* remove redundant UpsertBatchSpecInput method

Co-authored-by: Adam Harvey <adam@adamharvey.name>
Co-authored-by: Adam Harvey <adam@adamharvey.name>
* Cancel reading from stdin on Ctrl-C

This is essentially a copy-paste implementation of the ideas presented
in this comment here: golang/go#20280 (comment)

It fixes #775 and helps with the issue described in sourcegraph/src-cli#793 (comment). Not sure if it has unintended side-effects.

* Add comments and remove interface

* update changelog

Co-authored-by: BolajiOlajide <25608335+BolajiOlajide@users.noreply.github.com>
@BolajiOlajide BolajiOlajide self-assigned this Apr 16, 2024
@cla-bot
Copy link

cla-bot bot commented Apr 16, 2024

We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added.

Sourcegraph teammates should refer to Accepting contributions for guidance.


# We need buildx to be able to build a multi-architecture image.
- name: Set up Docker buildx
uses: docker/setup-buildx-action@v3

Check warning

Code scanning / Semgrep OSS

Semgrep Finding: security-semgrep-rules.semgrep-rules.yaml.third-party-action-not-pinned-to-commit-sha

An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
# We also need QEMU, since this is running on an AMD64 host and we want to
# build ARM64 images.
- name: Set up QEMU
uses: docker/setup-qemu-action@v3

Check warning

Code scanning / Semgrep OSS

Semgrep Finding: security-semgrep-rules.semgrep-rules.yaml.third-party-action-not-pinned-to-commit-sha

An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
with:
go-version: 1.20.x
- name: Check GoReleaser config
uses: goreleaser/goreleaser-action@v5

Check warning

Code scanning / Semgrep OSS

Semgrep Finding: security-semgrep-rules.semgrep-rules.yaml.third-party-action-not-pinned-to-commit-sha

An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }}
DOCKER_USERNAME: ${{ secrets.DOCKER_USERNAME }}
- name: Run GoReleaser
uses: goreleaser/goreleaser-action@v5

Check warning

Code scanning / Semgrep OSS

Semgrep Finding: security-semgrep-rules.semgrep-rules.yaml.third-party-action-not-pinned-to-commit-sha

An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
if o.Scripts.SourcegraphPrepublish == "" {
return nil
}
cmd := exec.Command("bash", "-c", o.Scripts.SourcegraphPrepublish)

Check failure

Code scanning / Semgrep OSS

Semgrep Finding: security-semgrep-rules.semgrep-rules.golang.detect-golang-exec-with-shell

Detected use of exec.Command invoking a shell. This can lead to command injection if inputs are not validated.
if *run {
for _, c := range commands {
out.WriteLine(output.Emojif(output.EmojiInfo, "Running command: %q", c))
command := exec.Command("bash", "-c", c)

Check failure

Code scanning / Semgrep OSS

Semgrep Finding: security-semgrep-rules.semgrep-rules.golang.detect-golang-exec-with-shell

Detected use of exec.Command invoking a shell. This can lead to command injection if inputs are not validated.
if *run {
for _, c := range commands {
out.WriteLine(output.Emojif(output.EmojiInfo, "Running command: %q", c))
command := exec.Command("bash", "-c", c)

Check failure

Code scanning / Semgrep OSS

Semgrep Finding: security-semgrep-rules.semgrep-rules.golang.detect-golang-exec-with-shell

Detected use of exec.Command invoking a shell. This can lead to command injection if inputs are not validated.
Comment on lines +204 to +205
cmd := exec.Command("bash", "-c", o.Scripts.SourcegraphPrepublish)
cmd.Env = append(os.Environ(), fmt.Sprintf("PATH=%s:%s", filepath.Join(dir, "node_modules", ".bin"), os.Getenv("PATH")))

Check warning

Code scanning / Semgrep OSS

Semgrep Finding: security-semgrep-rules.semgrep-rules.golang.detect-exec-cmd-with-arguments

Detected exec.Command without validating inputs.
@BolajiOlajide BolajiOlajide requested a review from a team April 16, 2024 12:10
@jhchabran
Copy link
Contributor

Thanks for taking on that @BolajiOlajide, btw what's the plan for the homebrew build?

@BolajiOlajide
Copy link
Contributor Author

Thanks for taking on that @BolajiOlajide, btw what's the plan for the homebrew build?

The plan is still to maintain it. Everything is automated by goreleaser in this github action: https://github.com/sourcegraph/src-cli/blob/main/.github/workflows/goreleaser.yml

The tap files live here: https://github.com/sourcegraph/homebrew-src-cli

Copy link
Contributor

@jhchabran jhchabran left a comment

Choose a reason for hiding this comment

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

thanks @BolajiOlajide for taking up on this. There's a bunch of nogo errors blocking CI though. Happy to help if you want me to.

@BolajiOlajide
Copy link
Contributor Author

thanks @BolajiOlajide for taking up on this. There's a bunch of nogo errors blocking CI though. Happy to help if you want me to.

I've got it. Today started really slow so I couldn't get to it in time. I'll reach out if there's anything I need help with.

@cla-bot
Copy link

cla-bot bot commented Apr 17, 2024

We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added.

Sourcegraph teammates should refer to Accepting contributions for guidance.

@cla-bot
Copy link

cla-bot bot commented Apr 22, 2024

We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added.

Sourcegraph teammates should refer to Accepting contributions for guidance.

@cla-bot
Copy link

cla-bot bot commented Apr 22, 2024

We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added.

Sourcegraph teammates should refer to Accepting contributions for guidance.

@cla-bot cla-bot bot added the cla-signed label Apr 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.