[test] Ensure image builds preserve environment variables #5609
Conversation
| DockerfilePath: ".gitpod.Dockerfile", | ||
| ContextPath: ".", | ||
| Source: &csapi.WorkspaceInitializer{ | ||
| Spec: &csapi.WorkspaceInitializer_Git{ |
There was a problem hiding this comment.
Relying on external, pre-existing fixtures is bound to break. I'd strongly vote for trying to keep these tests self-contained.
We have a content initializer which can download files from a URL. We could use GCP storage to upload the file, produce a pre-signed URL and use that to init the workspace content. Alternatively we should use GitHub API/git to set up the repo before we use it.
There was a problem hiding this comment.
We have a content initializer which can download files from a URL.
Had the same line of thought, but ended it with "file download is equivalent to git clone". The idea with uploading the content upfront is nice, but then we should use sth that is available to all Gitpod installations (abuse content-service?). Also, we kind of break abstractions because we make assumptions about the internal structure of a workspace backup.
Alternatively we could have a variant of the FileDownloadInitializer/a new DirecContentInitializer which allows to pass bytes of a .tar.gz directly into the initializer spec. This would be awesome for testing. WDYT? @csweichel
There was a problem hiding this comment.
I added FilesInitializer to see how it turns out - and it makes testing much more wholesome. 🤗
No hard feelings about names or impl but I like the idea.
Continuing on this: To even more encapsulate theses tests from GitHub especially we could have an CreateWorkspace API that takes an already parsed context (for testing only?). Context parsing itself should be handled by unit tests, so we don't loose anything by taking it out of the loop. Plus we achieve "full GitHub independence". 🙂
There was a problem hiding this comment.
o even more encapsulate theses tests from GitHub especially we could have an CreateWorkspace API that takes an already parsed context (for testing only?)
Thinking a bit more about it that does not make sense: We still rely on external services after context parsing (config deduction). Moving further down the chain calling WorkspaceStarter directly would be nice - and would eliminate the need for the "to-image-build-or-not" in the integration test suite. 😬
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Associated issue: #5497 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
Currently blocked by: #5497
Related Issue(s)
Ensures #5597 does not happen again.
How to test
cd /workspace/gitpod/test/tests/workspace && go test -run TestImageBuildPreservesEnvVar ./...Release Notes