Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

change: allow different protocols on the same port (manager#1566) #2361

Closed
wants to merge 5 commits into from

Conversation

iwilltry42
Copy link
Contributor

Ref https://github.com/acorn-io/manager/issues/1566

With this, it's OK to expose the same port/targetPort combination as long as they're using different protocols (which are non-blocking).

Checklist

  • The title of this PR would make a good line in Acorn's Release Note's Changelog
  • The title of this PR ends with a link to the main issue being address in parentheses, like: This is a title (#1216). Here's an example
  • All relevant issues are referenced in the PR description. NOTE: don't use GitHub keywords that auto-close issues
  • Commits follow contributing guidance
  • Automated tests added to cover the changes. If tests couldn't be added, an explanation is provided in the Verification and Testing section
  • Changes to user-facing functionality, API, CLI, and upgrade impacts are clearly called out in PR description
  • PR has at least two approvals before merging (or a reasonable exception, like it's just a docs change)

pkg/ports/publish.go Outdated Show resolved Hide resolved
@iwilltry42
Copy link
Contributor Author

I feel like this should throw an error/warning way earlier in the process instead of just dropping some ports without further notice. Any opinions on this?

@iwilltry42 iwilltry42 requested a review from vincent99 December 7, 2023 05:17
Copy link
Contributor

@keyallis keyallis left a comment

Choose a reason for hiding this comment

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

Generally looks good, requested a logic change and potentially another test case

pkg/ports/publish.go Outdated Show resolved Hide resolved
pkg/ports/ports_test.go Show resolved Hide resolved
Copy link
Contributor

@tylerslaton tylerslaton left a comment

Choose a reason for hiding this comment

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

Approving with the assumption that @keyallis's feedback is addressed. That's the only thing blocking in my mind here.

@iwilltry42 iwilltry42 requested a review from keyallis February 12, 2024 10:43
@iwilltry42
Copy link
Contributor Author

Fixed the logic to match what we discussed before - added related comments.
I also had to update quite a bunch of other files to make the revive linter happy 😬

@cjellick cjellick closed this Feb 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants