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.

Sign up for GitHub

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

Jump to bottom

feat: ability to override admin console and lam host ports #1201

Merged
merged 5 commits into from
Sep 20, 2024
Merged

feat: ability to override admin console and lam host ports #1201

merged 5 commits into from
Sep 20, 2024

Conversation

emosbaugh
Copy link
Member

@emosbaugh emosbaugh commented Sep 18, 2024 •
edited
Loading

What this PR does / why we need it:

  • Adds the ability to override the Admin Console port (default 30000) on installation using the flag --admin-console-port
  • Adds the ability to override the Local Artifact Mirror port (default 50000) on installation using the flag --local-artifact-mirror-port
  • Adds the ability to override the Local Artifact Mirror port (default 50000) on restore using the flag --local-artifact-mirror-port

Which issue(s) this PR fixes:

Does this PR require a test?

Does this PR require a release note?

- Adds the ability to override the Admin Console port (default 30000) on installation using the flag `--admin-console-port`
- Adds the ability to override the Local Artifact Mirror port (default 50000) on installation using the flag `--local-artifact-mirror-port`
- Adds the ability to override the Local Artifact Mirror port (default 50000) on restore using the flag `--local-artifact-mirror-port`

Does this PR require documentation?

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 18, 2024 •
edited
Loading

This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID.

Online Installer:

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci/appver-dev-c15f242" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Airgap Installer (may take a few minutes before the airgap bundle is built):

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci-airgap/appver-dev-c15f242?airgap=true" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Happy debugging!

@emosbaugh emosbaugh force-pushed the emosbaugh/sc-112092/provide-a-way-to-relocate-the-admin-console branch from 33e4c22 to f8b2d50 Compare September 19, 2024 16:45
ajp-io
ajp-io reviewed Sep 19, 2024
View reviewed changes
cmd/embedded-cluster/flags.go
Comment on lines +36 to +37
Name: "local-artifact-mirror-port",
Usage: "Port on which the Local Artifact Mirror will be served",
Copy link
Member

Choose a reason for hiding this comment

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

I was considering whether it was worth mentioning the name LAM or not, because an end user will have no idea what that is and doesn't need to. But I don't know what else we'd call it other than port-2. So maybe it's fine.

I don't have a strong preference, but we could consider lam-port if we feel like local-artifact-mirror-port is too long, or use it as an alias. People don't type this much, so I don't care much.

cmd/embedded-cluster/flags.go
return &cli.StringFlag{
Name: "admin-console-port",
Usage: "Port on which the Admin Console will be served",
Value: strconv.Itoa(defaults.AdminConsolePort),
Copy link
Member

Choose a reason for hiding this comment

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

Nice. Came here looking for this because I like the default value to show in the help menu so users can more easily determine if they need to set it to something else.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a range of ports that that are valid that we should validate this against? Maybe 80-32767 or whatever the node port range is set to?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see your TODO down lower

@emosbaugh emosbaugh force-pushed the emosbaugh/sc-112092/provide-a-way-to-relocate-the-admin-console branch 2 times, most recently from 9381573 to f231713 Compare September 19, 2024 20:31
@emosbaugh emosbaugh force-pushed the emosbaugh/sc-112092/provide-a-way-to-relocate-the-admin-console branch from f231713 to edbded1 Compare September 19, 2024 20:34
@emosbaugh emosbaugh marked this pull request as ready for review September 20, 2024 16:21
sgalsaleh
sgalsaleh reviewed Sep 20, 2024
View reviewed changes
Makefile
@@ -282,7 +283,7 @@ create-node%:
-v /var/lib/k0s \
-v $(shell pwd):/replicatedhq/embedded-cluster \
-v $(shell dirname $(shell pwd))/kots:/replicatedhq/kots \
$(if $(filter node0,node$*),-p 30000:30000) \
$(if $(filter node0,node$*),-p $(NODE_PORT):$(NODE_PORT)) \
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the container port be 30000? Otherwise you'd have to pass the admin console port flag right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes this is intentional

sgalsaleh
sgalsaleh reviewed Sep 20, 2024
View reviewed changes
cmd/embedded-cluster/restore.go
&cli.StringFlag{
Name: "local-artifact-mirror-port",
Usage: "Port on which the Local Artifact Mirror will be served. If left empty, the port will be retrieved from the snapshot.",
// DefaultText: strconv.Itoa(defaults.LocalArtifactMirrorPort),
Copy link
Member

Choose a reason for hiding this comment

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

is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes im not sure what is right here

@emosbaugh emosbaugh merged commit c6a57a4 into main Sep 20, 2024
56 checks passed
@emosbaugh emosbaugh deleted the emosbaugh/sc-112092/provide-a-way-to-relocate-the-admin-console branch September 20, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajp-io ajp-io ajp-io left review comments

@sgalsaleh sgalsaleh sgalsaleh approved these changes

Assignees
No one assigned
Labels
type::feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emosbaugh @ajp-io @sgalsaleh