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.

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

feat: add build arguments to build page #7253

Merged
merged 1 commit into from
May 27, 2024

Conversation

cdrage
Copy link
Contributor

@cdrage cdrage commented May 17, 2024

feat: add build arguments to build page

What does this PR do?

Build arguments is already built-in to podman desktop API.

This adds to the UI the ability to pass in build arguments.

Screenshot / video of UI

Screen.Recording.2024-05-17.at.4.15.22.PM.mov

What issues does this PR fix or reference?

Closes #7250

How to test this PR?

  • Tests are covering the bug fix or the new feature
  1. Build a Containerfile with ARG
  2. See it fail (normal)
  3. In build args, add your arg (ex. foo=bar)
  4. It will now build and pass.

@cdrage cdrage requested review from benoitf and a team as code owners May 17, 2024 20:18
@cdrage cdrage requested review from dgolovin and axel7083 and removed request for a team May 17, 2024 20:18
@cdrage cdrage force-pushed the add-args-build-image branch 2 times, most recently from 90c6f0c to fc78300 Compare May 17, 2024 20:31
@cdrage cdrage marked this pull request as draft May 17, 2024 20:54
@cdrage cdrage marked this pull request as ready for review May 21, 2024 13:14
@cdrage
Copy link
Contributor Author

cdrage commented May 21, 2024

@odockal Need your help on the failure if that's okay! I've been trying locally to get it "not strict" button selection?

⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯

 FAIL  tests/playwright/src/specs/image-smoke.spec.ts > Image workflow verification > Build image
Error: locator._expect: Error: strict mode violation: getByRole('button', { name: 'Build' }) resolved to 3 elements:
    1) <button disabled type="button" aria-label="Delete build …>…</button> aka getByLabel('Delete build argument')
    2) <button type="button" title="Add build argument" class="…>…</button> aka getByRole('button', { name: 'Add build argument' })
    3) <button type="button" class="relative px-4 py-[5px] box-…>…</button> aka getByRole('button', { name: 'Build', exact: true })

Call log:
  - locator._expect with timeout 5000ms
  - waiting for getByRole('button', { name: 'Build' })

 ❯ BuildImagePage.buildImage tests/playwright/src/model/pages/build-image-page.ts:88:40
     86|     }
     87| 
     88|     await playExpect(this.buildButton).toBeEnabled();
       |                                        ^
     89|     await this.buildButton.click();
     90|     await playExpect(this.doneButton).toBeEnabled({ timeout: 120000 });

@odockal
Copy link
Contributor

odockal commented May 22, 2024

@cdrage You need to add strict check into the locator since you have added new buttons that introduce an ambiguity.

this.buildButton = page.getByRole('button', { name: 'Build' });

Just add strict: true: this.buildButton = page.getByRole('button', { name: 'Build', strict: true });

@cdrage cdrage requested a review from a team as a code owner May 22, 2024 12:53
@cdrage
Copy link
Contributor Author

cdrage commented May 22, 2024

@cdrage You need to add strict check into the locator since you have added new buttons that introduce an ambiguity.

this.buildButton = page.getByRole('button', { name: 'Build' });

Just add strict: true: this.buildButton = page.getByRole('button', { name: 'Build', strict: true });

Updated!

PR is ready for review 💯 @axel7083 @benoitf @dgolovin

@odockal
Copy link
Contributor

odockal commented May 22, 2024

Note: correct is { name: 'Build', exact: true }.

### What does this PR do?

Build arguments is already built-in to podman desktop API.

This adds to the UI the ability to pass in build arguments.

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#7250

### How to test this PR?

<!-- Please explain steps to verify the functionality,
do not forget to provide unit/component tests -->

- [X] Tests are covering the bug fix or the new feature

1. Build a Containerfile with ARG
2. See it fail (normal)
3. In build args, add your arg (ex. foo=bar)
4. It will now build and pass.

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@benoitf
Copy link
Collaborator

benoitf commented May 27, 2024

I don't know if long term maybe we should have an 'advanced section' or tab to separate actions that are optionals

Also I think we discussed to remember all the build options over run (to run again the same build later)

(not preventing the merge of this PR, just highlighting some previous discussions)

@cdrage
Copy link
Contributor Author

cdrage commented May 27, 2024

I don't know if long term maybe we should have an 'advanced section' or tab to separate actions that are optionals

Also I think we discussed to remember all the build options over run (to run again the same build later)

(not preventing the merge of this PR, just highlighting some previous discussions)

100% agree.

I've created issue #7254 which is about automatically filling in those sections automatically.

I have opened up issue: #7308 about adding an advanced drop-down section / tab.

@cdrage cdrage merged commit ae38449 into containers:main May 27, 2024
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.11.0 milestone May 27, 2024
cdrage added a commit to cdrage/podman-desktop that referenced this pull request May 27, 2024
### What does this PR do?

Fixes the tests as the PR
containers#7253 wasn't rebased
before merging and it has a failing test.

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

N/A

### How to test this PR?

<!-- Please explain steps to verify the functionality,
do not forget to provide unit/component tests -->

- [X] Tests are covering the bug fix or the new feature

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
cdrage added a commit that referenced this pull request May 27, 2024
### What does this PR do?

Fixes the tests as the PR
#7253 wasn't rebased
before merging and it has a failing test.

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

N/A

### How to test this PR?

<!-- Please explain steps to verify the functionality,
do not forget to provide unit/component tests -->

- [X] Tests are covering the bug fix or the new feature

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
cdrage added a commit to cdrage/podman-desktop that referenced this pull request Jun 19, 2024
### What does this PR do?

Build arguments is already built-in to podman desktop API.

This adds to the UI the ability to pass in build arguments.

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#7250

### How to test this PR?

<!-- Please explain steps to verify the functionality,
do not forget to provide unit/component tests -->

- [X] Tests are covering the bug fix or the new feature

1. Build a Containerfile with ARG
2. See it fail (normal)
3. In build args, add your arg (ex. foo=bar)
4. It will now build and pass.

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
cdrage added a commit to cdrage/podman-desktop that referenced this pull request Jun 19, 2024
### What does this PR do?

Fixes the tests as the PR
containers#7253 wasn't rebased
before merging and it has a failing test.

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

N/A

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

N/A

### How to test this PR?

<!-- Please explain steps to verify the functionality,
do not forget to provide unit/component tests -->

- [X] Tests are covering the bug fix or the new feature

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ARG / passing in arguments for image building.
5 participants