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: show warnings on pod creation and two containers use the same port (#2240) #2671

Merged
merged 4 commits into from
Jun 1, 2023

Conversation

lstocchi
Copy link
Contributor

@lstocchi lstocchi commented May 29, 2023

What does this PR do?

This PR proposes a way to guide users in case they try to create a pod with multiple containers and two or more of them use the same port

Screenshot/screencast of this PR

same-port-containers

What issues does this PR fix or reference?

it resolves #2240

How to test this PR?

  1. select two containers using the same port
  2. create a pod from them
  3. in the create pod page you should see the warning

@lstocchi lstocchi requested review from a team and benoitf as code owners May 29, 2023 10:50
@lstocchi lstocchi requested review from jeffmaury and cdrage and removed request for a team May 29, 2023 10:50
@deboer-tim
Copy link
Collaborator

+1 on the function. I don't love banner warning UI and it doesn't match anything else (because we don't have a similar issue anywhere else yet), but we could always get design input and change it later.

@mairin
Copy link
Member

mairin commented May 30, 2023

I see where you got the design idea for the Possible Runtime Error alert message... it looks like the Patternfly alert, but it's too bright I think for our theme. I created one of these for the OpenShift Local mockup, looks like this:

Screenshot from 2023-05-30 14-46-15

I think a similar look could work a little better here. (I will mock smtg up, will post later today)

@mairin
Copy link
Member

mairin commented May 30, 2023

Screenshot from 2023-05-30 16-33-00

What do you think about this? Here's the details:

  • fill: charcoal-600
  • yellow highlight color + header text color: amber-400
  • yellow icon color: amber-500
  • icon: font awesome triangle-exclamation
  • font size: 14pt

@lstocchi
Copy link
Contributor Author

I see where you got the design idea for the Possible Runtime Error alert message... it looks like the Patternfly alert

I copied the alert notification from the tailwindcss page https://v1.tailwindcss.com/components/alerts#left-accent-border

What do you think about this? Here's the details:

Much much better, thanks!! I'll update the PR to this 🙏

@jeffmaury
Copy link
Contributor

If there are several ports in conflicts, the message is:

image

Can we switch to:

Containers a and b use the same ports c and d ?

Signed-off-by: lstocchi <lstocchi@redhat.com>
@lstocchi
Copy link
Contributor Author

Containers a and b use the same ports c and d ?

Done. You can see the result in the gif https://user-images.githubusercontent.com/49404737/242282201-241ee8e5-129f-413d-9009-9e1dcfcf78a8.gif

@mairin
Copy link
Member

mairin commented May 31, 2023

That looks fantastic @lstocchi!!!

@jeffmaury
Copy link
Contributor

I had 2 containers:

  • one was using ports 8080 and 5000
  • second one using 8080 and 8443

First was stopped and second running.

Here's what I got:

image

Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
@lstocchi
Copy link
Contributor Author

lstocchi commented Jun 1, 2023

image

Ops my bad, i only focussed to see the results with multiple ports and i didn't check in other cases. Added another test for that. Please try again.

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.

LGTM

Copy link
Contributor

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Thanks @lstocchi was able to test this out and it warned correctly when using the same port against a wordpress + another wordpress container + mysql setup.

Code looks good too.

LGTM!

@cdrage cdrage merged commit 235aec1 into containers:main Jun 1, 2023
7 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.1.0 milestone Jun 1, 2023
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.

Containers created inside pod are exposing ports from all containers
6 participants