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

Allow k3s to customize apiServerPort on helm-controller #7834

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

StrongMonkey
Copy link
Contributor

@StrongMonkey StrongMonkey commented Jun 29, 2023

Proposed Changes

When using helm-controller with bootstrap option, the apiServer port is hard-coded to 6443. This means that if you are using a port other than 6443 for api-server, it will be broken. Instead, it should be parameterized and pass down apiServerPort config to helm controller

Helm-controller PR: k3s-io/helm-controller#198

Types of Changes

Verification

Testing

Linked Issues

#7835

User-Facing Change


Further Comments

@StrongMonkey StrongMonkey requested a review from a team as a code owner June 29, 2023 00:04
go.mod Outdated
@@ -16,6 +16,8 @@ replace (
github.com/golang/protobuf => github.com/golang/protobuf v1.5.3
github.com/googleapis/gax-go/v2 => github.com/googleapis/gax-go/v2 v2.1.1
github.com/juju/errors => github.com/k3s-io/nocode v0.0.0-20200630202308-cb097102c09f

github.com/k3s-io/helm-controller => github.com/acorn-io/helm-controller v0.0.0-20230626182853-bb03cf954008
Copy link
Member

Choose a reason for hiding this comment

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

Why are we now replacing a k3s-io org repo with an external one?

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is just to test the change before k3s-io/helm-controller#198 is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be changed once the dependency PR is merged

Copy link
Member

@brandond brandond Jun 29, 2023

Choose a reason for hiding this comment

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

@StrongMonkey StrongMonkey force-pushed the v1.27.2+k3s1-acorn branch 2 times, most recently from 830a7e2 to 527cea0 Compare June 29, 2023 22:58
@brandond brandond requested a review from dereknola June 29, 2023 23:00
@brandond
Copy link
Member

LGTM, looks like you need to rebase and resolve a conflict though.

Signed-off-by: Daishan Peng <daishan@acorn.io>
@StrongMonkey
Copy link
Contributor Author

@brandond thanks for the quick approval, rebased 👍

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +31.67 🎉

Comparison is base (0809187) 19.85% compared to head (803b217) 51.52%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #7834       +/-   ##
===========================================
+ Coverage   19.85%   51.52%   +31.67%     
===========================================
  Files          83      143       +60     
  Lines        7686    14519     +6833     
===========================================
+ Hits         1526     7481     +5955     
+ Misses       5930     5852       -78     
- Partials      230     1186      +956     
Flag Coverage Δ
e2etests 49.35% <100.00%> (?)
inttests 44.50% <100.00%> (?)
unittests 19.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/server/server.go 58.45% <100.00%> (ø)

... and 117 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

3 participants