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: Reports warnings on failed kube deploy, fixes error out #3050

Merged
merged 3 commits into from Jul 6, 2023

Conversation

cdrage
Copy link
Contributor

@cdrage cdrage commented Jun 28, 2023

feat: Reports warnings on failed kube deploy, fixes error out

What does this PR do?

This updates the Play Kubernetes YAML section similar to the output of "Deploy to Kubernetes".

  • Reports warnings when encountering container errors when using Play
    Kubernetes
  • Fixes the error / warning output that was not appearing correctly.

Screenshot/screencast of this PR

Screen.Recording.2023-06-28.at.2.43.52.PM.mov

What issues does this PR fix or reference?

Closes #2621
Closes #3033

How to test this PR?

  1. Press Play Kubernetes
  2. Use the following YAML:
# Save the output of this file and use kubectl create -f to import
# it into Kubernetes.
#
# Created with podman-4.5.1
apiVersion: v1
kind: Pod
metadata:
  annotations:
    io.podman.annotations.ulimit: nofile=524288:524288,nproc=7248:7248
  creationTimestamp: "2023-06-27T17:47:37Z"
  labels:
    app: reverentrobinson-pod
  name: reverentrobinson-pod
spec:
  containers:
  - image: docker.io/cdrage/helloworld:latest
    name: reverentrobinson
    ports:
    - containerPort: 80
      hostPort: 9000
    tty: true
  1. Should appear correctly.
  2. Try deploying again / you'll now get the correct erroring / warning.

Signed-off-by: Charlie Drage charlie@charliedrage.com

@cdrage cdrage requested review from a team and benoitf as code owners June 28, 2023 19:14
@cdrage cdrage requested review from jeffmaury and lstocchi and removed request for a team June 28, 2023 19:14
@cdrage
Copy link
Contributor Author

cdrage commented Jun 28, 2023

TODO: Need to add tests (trying to figure out how to mock a .yaml being passed in as a selected file)

@benoitf
Copy link
Collaborator

benoitf commented Jun 28, 2023

@cdrage I think you can just mock window.openFileDialog and then you mock window.playKube as well

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

I thought lint was checked at commit time

@benoitf
Copy link
Collaborator

benoitf commented Jun 29, 2023

@jeffmaury it is unless you bypass git hooks :-) (-n, --[no-]verify option on git)

@cdrage
Copy link
Contributor Author

cdrage commented Jun 29, 2023

@jeffmaury it is unless you bypass git hooks :-) (-n, --[no-]verify option on git)

Haha yup, I had to bypass the hook so I can push out the code for those to test it out :) Lint is fixed now.

### What does this PR do?

* Reports warnings when encountering container errors when using Play
  Kubernetes
* Fixes the error / warning output that was not appearing correctly.

### Screenshot/screencast of this PR

<!-- Please include a screenshot or a screencast explaining what is doing this PR -->

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

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

Closes containers#2621
Closes containers#3033

### How to test this PR?

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage
Copy link
Contributor Author

cdrage commented Jun 29, 2023

Tests added 💯 ready for another review.

@cdrage cdrage dismissed jeffmaury’s stale review July 4, 2023 15:04

changes complete

@cdrage cdrage requested a review from jeffmaury July 4, 2023 15:40
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

By running your example i get the old error
Error: (HTTP code 500) server error - playing YAML file: adding pod to state: name "reverentrobinson-pod" is in use: pod already exists .

By debugging it, it gets in the catch and it skips your code. Do you have another yaml file to test it?

@cdrage
Copy link
Contributor Author

cdrage commented Jul 5, 2023

By running your example i get the old error Error: (HTTP code 500) server error - playing YAML file: adding pod to state: name "reverentrobinson-pod" is in use: pod already exists .

By debugging it, it gets in the catch and it skips your code. Do you have another yaml file to test it?

That's intentional? I believe you still have the old pod running when trying to deploy.

@lstocchi
Copy link
Contributor

lstocchi commented Jul 5, 2023

That's intentional? I believe you still have the old pod running when trying to deploy.

I thought I followed your instruction. Have i misunderstood them? 🤔

Click play, select the yaml, deploy, everything works fine, repeat to see the error. No?

@cdrage
Copy link
Contributor Author

cdrage commented Jul 5, 2023

That's intentional? I believe you still have the old pod running when trying to deploy.

I thought I followed your instruction. Have i misunderstood them? 🤔

Click play, select the yaml, deploy, everything works fine, repeat to see the error. No?

Yup! It's intentional. The error is appearing.

#3033 = It's showing the error.

#2621 = no more layout being broken.

Playing it twice induces the error and shows what this PR is fixing.

Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Ok I think i got it working by editing your yaml. Works fine, also the issue i saw with the broken layout is fixed.
Just a problem with a possible undefined value. For the rest looks good

packages/renderer/src/lib/kube/KubePlayYAML.svelte Outdated Show resolved Hide resolved
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage
Copy link
Contributor Author

cdrage commented Jul 6, 2023

@lstocchi Thanks @lstocchi ! can you check the newest commit? I've added a change to that single line of code.

Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

LGTM, works fine! Thanks @cdrage 🚀

@cdrage cdrage enabled auto-merge (squash) July 6, 2023 16:56
@cdrage cdrage merged commit 982888c into containers:main Jul 6, 2023
7 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.2.0 milestone Jul 6, 2023
mairin pushed a commit to mairin/podman-desktop that referenced this pull request Jul 7, 2023
…ners#3050)

* feat: Reports warnings on failed kube deploy, fixes error out

### What does this PR do?

* Reports warnings when encountering container errors when using Play
  Kubernetes
* Fixes the error / warning output that was not appearing correctly.

### Screenshot/screencast of this PR

<!-- Please include a screenshot or a screencast explaining what is doing this PR -->

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

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

Closes containers#2621
Closes containers#3033

### How to test this PR?

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>

* add tests

Signed-off-by: Charlie Drage <charlie@charliedrage.com>

* make sure we check to see if containererrors is undefined first

Signed-off-by: Charlie Drage <charlie@charliedrage.com>

---------

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
5 participants