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.

Sign up for GitHub

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

[test] Ensure image builds preserve environment variables #5609

Closed
wants to merge 3 commits into from
Closed

[test] Ensure image builds preserve environment variables #5609

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1dcb029
[test] Ensure image builds preserve environment variables
geropl Sep 8, 2021
793fc84
[content] FilesInitializer
geropl Sep 15, 2021
6d5b5c1
[test] adjust to FilesInitializer
geropl Sep 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 51 additions & 41 deletions test/pkg/integration/workspace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@ const (
)

type launchWorkspaceDirectlyOptions struct {
BaseImage string
IdeImage string
Mods []func(*wsmanapi.StartWorkspaceRequest) error
WaitForOpts []WaitForWorkspaceOpt
BaseImage string
WorkspaceImageRequest *imgbldr.ResolveWorkspaceImageRequest
ImgBldrOpts []APIImageBuilderOpt
IdeImage string
Mods []func(*wsmanapi.StartWorkspaceRequest) error
WaitForOpts []WaitForWorkspaceOpt
}

// LaunchWorkspaceDirectlyOpt configures the behaviour of LaunchWorkspaceDirectly
Expand All @@ -52,7 +54,7 @@ func WithoutWorkspaceImage() LaunchWorkspaceDirectlyOpt {

// WithBaseImage configures the base image used to start the workspace. The base image
// will be resolved to a workspace image using the image builder. If the corresponding
// workspace image isn't built yet, it will NOT be built.
// workspace image isn't built yet, it will be built.
func WithBaseImage(baseImage string) LaunchWorkspaceDirectlyOpt {
return func(lwdo *launchWorkspaceDirectlyOptions) error {
lwdo.BaseImage = baseImage
Expand Down Expand Up @@ -86,6 +88,22 @@ func WithWaitWorkspaceForOpts(opt ...WaitForWorkspaceOpt) LaunchWorkspaceDirectl
}
}

// WithWorkspaceImageRequest is a more complete alternative to WithBaseImage
func WithWorkspaceImageRequest(req *imgbldr.ResolveWorkspaceImageRequest) LaunchWorkspaceDirectlyOpt {
return func(lwdo *launchWorkspaceDirectlyOptions) error {
lwdo.WorkspaceImageRequest = req
return nil
}
}

// WithImageBuilderOpts allows to pass options to ImageBuilder(opts)
func WithImageBuilderOpts(opts ...APIImageBuilderOpt) LaunchWorkspaceDirectlyOpt {
return func(lwdo *launchWorkspaceDirectlyOptions) error {
lwdo.ImgBldrOpts = opts
return nil
}
}

// LaunchWorkspaceDirectlyResult is returned by LaunchWorkspaceDirectly
type LaunchWorkspaceDirectlyResult struct {
Req *wsmanapi.StartWorkspaceRequest
Expand Down Expand Up @@ -120,12 +138,29 @@ func LaunchWorkspaceDirectly(it *Test, opts ...LaunchWorkspaceDirectlyOpt) (res
}

var workspaceImage string
if options.BaseImage != "" {
workspaceImage, err = it.resolveOrBuildImage(options.BaseImage)
if err != nil {
it.t.Fatalf("cannot resolve base image: %v", err)
return
}
if options.WorkspaceImageRequest != nil {
workspaceImage, err = it.resolveOrBuildImage(options.WorkspaceImageRequest, options.ImgBldrOpts...)
} else if options.BaseImage != "" {
workspaceImage, err = it.resolveOrBuildImage(&imgbldr.ResolveWorkspaceImageRequest{
Source: &imgbldr.BuildSource{
From: &imgbldr.BuildSource_Ref{
Ref: &imgbldr.BuildSourceReference{
Ref: options.BaseImage,
},
},
},
Auth: &imgbldr.BuildRegistryAuth{
Mode: &imgbldr.BuildRegistryAuth_Total{
Total: &imgbldr.BuildRegistryAuthTotal{
AllowAll: true,
},
},
},
}, options.ImgBldrOpts...)
}
if err != nil {
it.t.Fatalf("cannot resolve base image: %v", err)
return
}

ideImage := options.IdeImage
Expand Down Expand Up @@ -512,25 +547,10 @@ func (it *Test) WaitForWorkspace(instanceID string, condition func(status *wsman
}
}

func (it *Test) resolveOrBuildImage(baseRef string) (absref string, err error) {
func (it *Test) resolveOrBuildImage(req *imgbldr.ResolveWorkspaceImageRequest, imgbldrOpts ...APIImageBuilderOpt) (absref string, err error) {
rctx, rcancel := context.WithTimeout(it.ctx, perCallTimeout)
cl := it.API().ImageBuilder()
reslv, err := cl.ResolveWorkspaceImage(rctx, &imgbldr.ResolveWorkspaceImageRequest{
Source: &imgbldr.BuildSource{
From: &imgbldr.BuildSource_Ref{
Ref: &imgbldr.BuildSourceReference{
Ref: baseRef,
},
},
},
Auth: &imgbldr.BuildRegistryAuth{
Mode: &imgbldr.BuildRegistryAuth_Total{
Total: &imgbldr.BuildRegistryAuthTotal{
AllowAll: true,
},
},
},
})
cl := it.API().ImageBuilder(imgbldrOpts...)
reslv, err := cl.ResolveWorkspaceImage(rctx, req)
rcancel()
if err != nil {
return
Expand All @@ -546,19 +566,9 @@ func (it *Test) resolveOrBuildImage(baseRef string) (absref string, err error) {
defer rcancel()
bld, err := cl.Build(rctx, &imgbldr.BuildRequest{
Source: &imgbldr.BuildSource{
From: &imgbldr.BuildSource_Ref{
Ref: &imgbldr.BuildSourceReference{
Ref: baseRef,
},
},
},
Auth: &imgbldr.BuildRegistryAuth{
Mode: &imgbldr.BuildRegistryAuth_Total{
Total: &imgbldr.BuildRegistryAuthTotal{
AllowAll: true,
},
},
From: req.Source.From,
},
Auth: req.Auth,
})
if err != nil {
return
Expand Down
72 changes: 72 additions & 0 deletions test/tests/workspace/workspace_image_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Copyright (c) 2021 Gitpod GmbH. All rights reserved.
// Licensed under the GNU Affero General Public License (AGPL).
// See License-AGPL.txt in the project root for license information.

package workspace_test

import (
"strings"
"testing"
"time"

csapi "github.com/gitpod-io/gitpod/content-service/api"
imgapi "github.com/gitpod-io/gitpod/image-builder/api"
"github.com/gitpod-io/gitpod/test/pkg/integration"
agent "github.com/gitpod-io/gitpod/test/tests/workspace/workspace_agent/api"
"github.com/gitpod-io/gitpod/ws-manager/api"
)

func TestImageBuildPreservesEnvVarMk3(t *testing.T) {
it, ctx := integration.NewTest(t, 5*time.Minute)
defer it.Done()

res := integration.LaunchWorkspaceDirectly(it, integration.WithWorkspaceImageRequest(&imgapi.ResolveWorkspaceImageRequest{
Source: &imgapi.BuildSource{
From: &imgapi.BuildSource_File{
File: &imgapi.BuildSourceDockerfile{
DockerfileVersion: "some-version",
DockerfilePath: ".gitpod.Dockerfile",
ContextPath: ".",
Source: &csapi.WorkspaceInitializer{
Spec: &csapi.WorkspaceInitializer_Git{
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@geropl geropl Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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". 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

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. 😬

Git: &csapi.GitInitializer{
RemoteUri: "https://github.com/gitpod-io/gitpod-test-repo.git",
TargetMode: csapi.CloneTargetMode_REMOTE_BRANCH,
// this branch has a docker file that adds 'MY_TEST_ENV_VAR=asd' as env var (ref: https://github.com/gitpod-io/gitpod-test-repo/blob/integration-test/imgbldr/env-is-persisted/.gitpod.Dockerfile#L3)
CloneTaget: "integration-test/imgbldr/env-is-persisted",
Config: &csapi.GitConfig{
Authentication: csapi.GitAuthMethod_NO_AUTH,
},
},
},
},
},
},
},
}), integration.WithImageBuilderOpts(integration.SelectImageBuilderMK3))
defer it.API().WorkspaceManager().StopWorkspace(ctx, &api.StopWorkspaceRequest{
Id: res.Req.Id,
})

rsa, err := it.Instrument(integration.ComponentWorkspace, "workspace", integration.WithInstanceID(res.Req.Id))
if err != nil {
t.Fatal(err)
}
defer rsa.Close()

var resp agent.ExecResponse
err = rsa.Call("WorkspaceAgent.Exec", &agent.ExecRequest{
Dir: "/workspace",
Command: "bash",
Args: []string{"-c", "echo $MY_TEST_ENV_VAR"},
}, &resp)
if err != nil {
t.Fatal(err)
}
if resp.ExitCode != 0 {
t.Fatalf("got non-zero exit code: %d", resp.ExitCode)
}
if strings.TrimSpace(resp.Stdout) == "" {
t.Fatalf("env var MY_TEST_ENV_VAR is not preserved!")
}
}