Skip to content

HOSTEDCP-1285: Kas port svc cleanup - #3186

Merged
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
enxebre:kas-port-svc-cleanup
Nov 17, 2023
Merged

openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
enxebre:kas-port-svc-cleanup

Conversation

@enxebre

@enxebre enxebre commented Nov 13, 2023 •

Copy link
Copy Markdown
Member

What this PR does / why we need it:
The kas SVC port is an impl detail. Before this change their value was wrongly conflated with the spec.metworking.apiServer.port input.
If we ever require the dedicated KAS LB port to be a choice, that would be input in the LB publishing strategy.

Goes after #3185

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

@openshift-ci openshift-ci Bot added do-not-merge/needs-area do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 13, 2023
@openshift-ci

openshift-ci Bot commented Nov 13, 2023

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@enxebre

enxebre commented Nov 13, 2023

Copy link
Copy Markdown
Member Author

/hold
Goes after #3185

@enxebre
enxebre marked this pull request as ready for review November 13, 2023 12:52
@openshift-ci openshift-ci Bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 13, 2023
@enxebre enxebre changed the title Kas port svc cleanup HOSTEDCP-1285: Kas port svc cleanup Nov 13, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 13, 2023
@openshift-ci-robot

openshift-ci-robot commented Nov 13, 2023 •

Copy link
Copy Markdown

@enxebre: This pull request references HOSTEDCP-1285 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

Details

In response to this:

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot

openshift-ci-robot commented Nov 13, 2023 •

Copy link
Copy Markdown

@enxebre: This pull request references HOSTEDCP-1285 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

Details

In response to this:

What this PR does / why we need it:
The kas SVC port is an impl detail. Before this change their value was wrongly conflated with the spec.metworking.apiServer.port input.
If we ever require the dedicated KAS LB port to be a choice, that would be input in the LB publishing strategy.

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci Bot added area/cli Indicates the PR includes changes for CLI area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release and removed do-not-merge/needs-area labels Nov 13, 2023
@openshift-ci

openshift-ci Bot commented Nov 13, 2023

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2023
@openshift-ci-robot

openshift-ci-robot commented Nov 13, 2023 •

Copy link
Copy Markdown

@enxebre: This pull request references HOSTEDCP-1285 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

Details

In response to this:

What this PR does / why we need it:
The kas SVC port is an impl detail. Before this change their value was wrongly conflated with the spec.metworking.apiServer.port input.
If we ever require the dedicated KAS LB port to be a choice, that would be input in the LB publishing strategy.

Goes after #3185

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@enxebre
enxebre force-pushed the kas-port-svc-cleanup branch 3 times, most recently from 8a97ac8 to 540197f Compare November 14, 2023 19:29
@netlify

netlify Bot commented Nov 14, 2023 •

Copy link
Copy Markdown

✅ Deploy Preview for hypershift-docs ready!

Name Link
🔨 Latest commit dbbb951
🔍 Latest deploy log https://app.netlify.com/sites/hypershift-docs/deploys/6557148a524029000878fbe0
😎 Deploy Preview https://deploy-preview-3186--hypershift-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@enxebre
enxebre force-pushed the kas-port-svc-cleanup branch 4 times, most recently from 2022f61 to 512437f Compare November 15, 2023 14:33
@enxebre

enxebre commented Nov 15, 2023

Copy link
Copy Markdown
Member Author

/hold cancel

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2023
@csrwng

csrwng commented Nov 16, 2023

Copy link
Copy Markdown
Contributor

My only comment is that it would be good to have a constant for the Azure port even if it's a hack. It should make it easier to find all references when we do want to remove the hack.

Other than that, lgtm

@enxebre
enxebre force-pushed the kas-port-svc-cleanup branch from 512437f to 7b85a05 Compare November 16, 2023 15:30
@csrwng

csrwng commented Nov 16, 2023

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2023
The kas SVC port is an impl detail. Before this change their value was wrongly conflated with the spec.metworking.apiServer.port input.
If we ever require the dedicated KAS LB port to be a choice, that would be input in the LB publishing strategy.
@enxebre
enxebre force-pushed the kas-port-svc-cleanup branch from 7b85a05 to dbbb951 Compare November 17, 2023 07:21
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2023
@enxebre

enxebre commented Nov 17, 2023

Copy link
Copy Markdown
Member Author

/test e2e-kubevirt-aws-ovn

@openshift-ci

openshift-ci Bot commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

@enxebre: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@csrwng

csrwng commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2023
@openshift-merge-bot
openshift-merge-bot Bot merged commit 3f4fce6 into openshift:main Nov 17, 2023
@openshift-bot

Copy link
Copy Markdown

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-hypershift-container-v4.15.0-202311171651.p0.g3f4fce6.assembly.stream for distgit hypershift.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cli Indicates the PR includes changes for CLI area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants