-
Notifications
You must be signed in to change notification settings - Fork 264
Bug 1608828 - migrate generic-worker into monorepo #2262
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
Conversation
* replace docker library code with shells out to docker cli using exec.Command * commit dep changes * dereference exitError * disable checking ExitCode() which requires go1.12 * use ExitStatus and syscall to get exit code pre ExitError() which was added in go1.12 * Ran dep ensure * remove docker constraint from Gopkg.toml * add command logging to docker Execute() * pass GOPATH and current go binary when calling sudo in build.sh * lookup docker in path
…/owners instead of ami-base-name
…r output if they fail
Bug 1567073 - log all commands that are executed on the host
Bug 1559777 - support deploying generic-worker linux workers to EC2
Trivial logging changes
Bug 1568782 - error artifacts do not have a content type
Bug 1568471 - close HTTP response bodies when finished with them
Bug 1567632 - internally cache interactive username (lazily) on macOS
This reverts commit 798c3d0.
…pt is only approximate
win2016 worker_type: s/aamazon/amazon
…lient-go package names
…th more generic TestGarbageNamespaces
3fdea7a to
4f6cd82
Compare
|
Hi @djmitche, I've reduced this to 16 commits on top of the generic worker repository. I also rewrote the generic-worker history to remove the It is probably easier to browse through the commits in sequence than view the complete diff, since that includes the entire generic-worker codebase. Note, I haven't tackled updating the monorepo release process to publish generic-worker binaries, or integrated the build with So despite not having all the i's dotted and t's crossed yet, I think this should be in good enough shape for a first pass. Note, in the end I closed taskcluster/community-tc-config#195 in favour of an alternative approach. |
…ws/win32/api/userenv/nf-userenv-getuserprofiledirectoryw despite msdn docs saying it is allowed
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.
This looks good, and the per-line comments below are minor and/or easy to handle in a followup.
Another good followup will be to get version = ".." in main.go substituted appropriately in yarn release. That should be a pretty easy modification to infrastructure/tooling/src/release/tasks.js.
I did get some issues running go test locally.
^C(go1.13.7) (v12.14.1) dustin@lamport ~/p/taskcluster2 [add-generic-worker] $ go test -v ./clients/client-go/integrationtest/
=== RUN TestGarbageNamespaces
2020/02/07 15:19:09 Error: Get /api/index/v1/namespaces/garbage: unsupported protocol scheme ""
(and in general this seems to hang)
# github.com/taskcluster/taskcluster/v24/workers/generic-worker/gwconfig
workers/generic-worker/gwconfig/config.go:30:3: undefined: PublicEngineConfig
=== RUN TestLiveLog
--- FAIL: TestLiveLog (0.00s)
livelog_test.go:35: Could not initiate livelog process:
WARNING: Livelog terminated early with error 'exec: "livelog": executable file not found in $PATH' and output:
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x48 pc=0x69a261]
=== RUN TestTcProxy
--- FAIL: TestTcProxy (0.00s)
testutil.go:17: TASKCLUSTER_{CLIENT_ID,ACCESS_TOKEN,ROOT_URL env vars not set, but GW_SKIP_INTEGRATION_TESTS not set
FAIL
It'd be nice to remove the following, too (it didn't run because of the build failure above)
workers/generic-worker/main_test.go: t.Fatalf("Git revision could not be determined - got '%v' but expected to match regular expression '^[0-9a-f](40)$'\n"+
as that parameter should be enforced by the build/release mechanics and isn't required for testing.
The approach we've taken to ease the process for contributors is to make tests (yarn test or go test ./...) "just work" out of the box, even if that means skipping tests that need credentials. Then we set $NO_TEST_SKIP in CI to turn those skips into failures. That way a contributor can make changes and run the tests locally pretty easily, but we still get to see test failures in the PR if they have broken some test that was otherwise skipped -- and since most tests are unit tests with suitable fakes, that situation is fairly unusual. For example, the websocktunnel tests "just work" out of the box and would catch bugs in modifications to the expose package.
So I think the major thing I'd like to see before landing here is to make the above skip when necessary bits (credentials, livelog) aren't available, and to update the build config so that a "straight" go test ./... succeeds -- either with a null engine, or defaulting to the multiuser engine?
.taskcluster.yml
Outdated
| taskclusterProxy: true | ||
| mounts: | ||
| - content: | ||
| url: https://storage.googleapis.com/golang/go1.13.7.linux-amd64.tar.gz |
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.
It'd be good if this substituted ${go}, perhaps listing the go sha256 as a variable in the same $let at the top of the file. Then there's just one place to change the go version.
README.md
Outdated
| * [ui/src/components/Snackbar](ui/src/components/Snackbar#readme) | ||
| * [ui/src/components/SpeedDial](ui/src/components/SpeedDial#readme) | ||
| * [ui/src/components/StatusLabel](ui/src/components/StatusLabel#readme) | ||
| * [generic-worker](workers/generic-worker#readme) |
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.
The name here comes from the #-level heading at the top of the file, if you'd like to change it (e.g., to capitalize it)
|
|
||
| // imports inscrutably uses the old tc-client path, so fix that up. Note | ||
| // that this must be the full path to allow `yarn release` to update it | ||
| // correctly. |
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.
This was a bug in goimports, but does indeed seem to be fixed now. Maybe it just found the other library in some GOPATH somewhere and that GOPATH has since been deleted? 🤷♀️
| PublicConfig struct { | ||
| PublicEngineConfig | ||
| AuthBaseURL string `json:"authBaseURL"` | ||
| AuthRootURL string `json:"authRootURL"` |
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.
I think this will require a README update as well, and it would be good to mention that the *RootURL configs are only for use in testing and development.
…cf427c431/src/cmd/compile/internal/gc/lex.go#L80-L90 - fixes intermittent DLL call wrapper failures due to allocated objects being moved by garbage collector before call completes
563106f to
f474bdf
Compare
c8237d8 to
1fd0571
Compare
Bugzilla Bug: 1608828
This initial PR integrates the generic-worker repository wholesale into the monorepo under /workers/generic-worker. The build scripts, CI tasks, package imports are updated as necessary, but the project is not restructured in any way. Future PRs can restructure or reorganise code as appropriate.
This is initially a draft pull request until I have everything working.
Git history from the generic-worker repository has been preserved.