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

Support -insecure-skip-verify flag for LSIF upload #559

Merged
merged 3 commits into from
Jun 25, 2021

Conversation

Strum355
Copy link
Contributor

@Strum355 Strum355 commented Jun 25, 2021

As we previously used a different API client as was provided in the src-cli codebase, we did not take into account the -skip-insecure-verify flag. This affected a customer as they used self-signed certs.

This PR passes the src-cli configured API client that honours the flag to the LSIF upload function that resides in sg/sg/lib. The only part of the API client that is consumed is the direct pass-through method, Do(...)

Related to https://github.com/sourcegraph/sourcegraph/pull/22399 and will require bumping go.mod here when it is merged.

cc @efritz for 👀 after PTO

@Strum355
Copy link
Contributor Author

Strum355 commented Jun 25, 2021

Note tests are failing due to go.mod pointing at the old sg/sg/lib API while the related PR is unmerged, will be rerun once it is merged

client := api.NewClient(api.ClientOpts{
Flags: lsifUploadFlags.apiFlags,
})

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a good idea to log a warning if the client is initialized without TLS validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a good idea, I'll follow up with a separate PR for that, as those changes would be more wide-reaching across teams 🙂

@Strum355 Strum355 merged commit 7051ea1 into main Jun 25, 2021
@Strum355 Strum355 deleted the nsc/lsif-upload-insecureskipverify branch June 25, 2021 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants