Skip to content

Commit

Permalink
fix: allow editing of build containerfile
Browse files Browse the repository at this point in the history
Changes the build image form from a readonly text field that you click on to
browser, to a more common editable text field with a Browse button. Likewise
the build directory was hidden until you entered a containerfile before, now
it is always visible and uses the same text field + browse button.

This change makes the form use a more common pattern, and allows e2e tests
to more easily enter text in either field.

The drawback to this approach is that there is no more validation than
before - the build button enables whenever there is text in both fields.
This happens automatically when you use the Browse button like before,
but if you edit or copy/paste and have a typo you won't find out until you
click Build. We could add more validation in the future, but IMHO the
better usability/form layout is enough of a benefit to make this change in
the meantime.

Signed-off-by: Tim deBoer <git@tdeboer.ca>
  • Loading branch information
deboer-tim committed Oct 24, 2023
1 parent dee09a2 commit 09583b7
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 26 deletions.
Expand Up @@ -88,9 +88,12 @@ test('Expect Build button is enabled', async () => {
render(BuildImageFromContainerfile, {});

const containerFilePath = screen.getByRole('textbox', { name: 'Containerfile Path' });

expect(containerFilePath).toBeInTheDocument();
await userEvent.click(containerFilePath);
await userEvent.type(containerFilePath, '/somepath/containerfile');

const buildFolder = screen.getByRole('textbox', { name: 'Build Context Directory' });
expect(buildFolder).toBeInTheDocument();
await userEvent.type(buildFolder, '/somepath');

const buildButton = screen.getByRole('button', { name: 'Build' });
expect(buildButton).toBeInTheDocument();
Expand All @@ -102,9 +105,12 @@ test('Expect Done button is enabled once build is done', async () => {
render(BuildImageFromContainerfile, {});

const containerFilePath = screen.getByRole('textbox', { name: 'Containerfile Path' });

expect(containerFilePath).toBeInTheDocument();
await userEvent.click(containerFilePath);
await userEvent.type(containerFilePath, '/somepath/containerfile');

const buildFolder = screen.getByRole('textbox', { name: 'Build Context Directory' });
expect(buildFolder).toBeInTheDocument();
await userEvent.type(buildFolder, '/somepath');

const buildButton = screen.getByRole('button', { name: 'Build' });
expect(buildButton).toBeInTheDocument();
Expand Down
48 changes: 26 additions & 22 deletions packages/renderer/src/lib/image/BuildImageFromContainerfile.svelte
Expand Up @@ -27,7 +27,6 @@ let buildFinished = false;
let containerImageName = 'my-custom-image';
let containerFilePath: string;
let containerBuildContextDirectory: string;
let hasInvalidFields = true;
let buildImageInfo: BuildImageInfo | undefined = undefined;
let providers: ProviderInfo[] = [];
Expand All @@ -36,6 +35,8 @@ let selectedProvider: ProviderContainerConnectionInfo | undefined = undefined;
let selectedProviderConnection: ProviderContainerConnectionInfo | undefined = undefined;
let logsTerminal: Terminal;
$: hasInvalidFields = !containerFilePath || !containerBuildContextDirectory;
function getTerminalCallback(): BuildImageCallback {
return {
onStream: function (data: string): void {
Expand Down Expand Up @@ -117,7 +118,6 @@ async function getContainerfileLocation() {
const result = await window.openFileDialog('Select Containerfile to build');
if (!result.canceled && result.filePaths.length === 1) {
containerFilePath = result.filePaths[0];
hasInvalidFields = false;
if (!containerBuildContextDirectory) {
// select the parent directory of the file as default
// eslint-disable-next-line no-useless-escape
Expand Down Expand Up @@ -146,28 +146,32 @@ async function getContainerBuildContextDirectory() {
<div class="bg-charcoal-900 pt-5 space-y-6 px-8 sm:pb-6 xl:pb-8 rounded-lg">
<div hidden="{buildImageInfo?.buildRunning}">
<label for="containerFilePath" class="block mb-2 text-sm font-bold text-gray-400">Containerfile Path</label>
<input
on:click="{() => getContainerfileLocation()}"
name="containerFilePath"
id="containerFilePath"
bind:value="{containerFilePath}"
readonly
placeholder="Select Containerfile to build..."
class="w-full p-2 outline-none text-sm bg-charcoal-600 rounded-sm text-gray-700 placeholder-gray-700"
required />
<div class="flex flex-row">
<input
name="containerFilePath"
id="containerFilePath"
bind:value="{containerFilePath}"
placeholder="Containerfile to build"
class="w-full p-2 mr-3 outline-none text-sm bg-charcoal-600 rounded-sm text-gray-700 placeholder-gray-700"
required />

<Button on:click="{() => getContainerfileLocation()}">Browse...</Button>
</div>
</div>

<div hidden="{!containerFilePath || buildImageInfo?.buildRunning}">
<div hidden="{buildImageInfo?.buildRunning}">
<label for="containerBuildContextDirectory" class="block mb-2 text-sm font-bold text-gray-400"
>Build context directory</label>
<input
on:click="{() => getContainerBuildContextDirectory()}"
name="containerBuildContextDirectory"
id="containerBuildContextDirectory"
bind:value="{containerBuildContextDirectory}"
readonly
class="w-full p-2 outline-none text-sm bg-charcoal-600 rounded-sm text-gray-700 placeholder-gray-700"
required />
>Build Context Directory</label>
<div class="flex flex-row">
<input
name="containerBuildContextDirectory"
id="containerBuildContextDirectory"
bind:value="{containerBuildContextDirectory}"
placeholder="Folder to build in"
class="w-full p-2 mr-3 outline-none text-sm bg-charcoal-600 rounded-sm text-gray-700 placeholder-gray-700"
required />
<Button on:click="{() => getContainerBuildContextDirectory()}">Browse...</Button>
</div>
</div>

<div hidden="{buildImageInfo?.buildRunning}">
Expand All @@ -177,7 +181,7 @@ async function getContainerBuildContextDirectory() {
bind:value="{containerImageName}"
name="containerImageName"
id="containerImageName"
placeholder="Enter image name (e.g. quay.io/namespace/my-custom-image)"
placeholder="image name (e.g. quay.io/namespace/my-custom-image)"
class="w-full p-2 outline-none text-sm bg-charcoal-600 rounded-sm text-gray-700 placeholder-gray-700"
required />
{#if providerConnections.length > 1}
Expand Down

0 comments on commit 09583b7

Please sign in to comment.