-
Notifications
You must be signed in to change notification settings - Fork 1.3k
codeintel: first implementation of auto-indexing secrets #45580
Conversation
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits 52df392 and 3cb4a50 or learn more. Open explanation
|
migrations/frontend/1670600028_executor_secrets_accesslogs_codeintel_user/up.sql
Outdated
Show resolved
Hide resolved
enterprise/cmd/frontend/internal/executorqueue/queues/codeintel/transform.go
Show resolved
Hide resolved
a478de9
to
171b493
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, good stuff!
enterprise/cmd/frontend/internal/executorqueue/queues/codeintel/transform_test.go
Show resolved
Hide resolved
enterprise/internal/codeintel/autoindexing/internal/inference/lua/typescript.lua
Outdated
Show resolved
Hide resolved
enterprise/internal/codeintel/autoindexing/internal/jobselector/job_selector.go
Outdated
Show resolved
Hide resolved
migrations/frontend/1670600028_executor_secrets_accesslogs_codeintel_user/up.sql
Outdated
Show resolved
Hide resolved
migrations/frontend/1670600028_executor_secrets_accesslogs_codeintel_user/up.sql
Show resolved
Hide resolved
enterprise/cmd/frontend/internal/executorqueue/queues/codeintel/transform.go
Show resolved
Hide resolved
ALTER TABLE executor_secret_access_logs | ||
DROP COLUMN IF EXISTS machine_user; | ||
|
||
DELETE FROM executor_secret_access_logs WHERE user_id IS NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evict Im not sure how desirable this would be. Any opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think we should keep existing access logs even though user_id
is not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobheadxi is there an approach we should take in the down migration here? Im not sure how we can preserve it without losing the machine_user
info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's move it to a backup table for worst-case scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep thats the approach im going to go with, will have an impl for review today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented here, second+ pair of eyes appreciated https://github.com/sourcegraph/sourcegraph/pull/45580/commits/46b40fd3383fc522c77fe15cf4e54d3dae063ca6
47f29d7
to
35f0b0c
Compare
Codenotify: Notifying subscribers in CODENOTIFY files for diff 3774fbd...52df392.
|
35f0b0c
to
851e093
Compare
client/web/src/enterprise/executors/secrets/ExecutorSecretsListPage.tsx
Outdated
Show resolved
Hide resolved
97ce807
to
a04a1ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice SQL trickery!
a04a1ea
to
7f185a6
Compare
Adds executor secrets support to auto-indexing, allowing for new capabilities including, and most notably, authentication with private registries. This is an early-phase (but functional) PR with first-step support for private Go modules.
Test plan
Ran a local E2E test-run (not an actual E2E automated test), back-compat tests keeping the DB stuff relatively sane, but also stressed importance of reviewing that as well as running it through a few scenarios
main dry-run https://buildkite.com/sourcegraph/sourcegraph/builds/189673