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

[mesheryctl] Bug fixes in mesheryctl system start #10961

Merged
merged 7 commits into from
May 17, 2024

Conversation

MUzairS15
Copy link
Contributor

@MUzairS15 MUzairS15 commented May 15, 2024

Notes for Reviewers

This PR fixes #

  1. mesheryctl system start command is run using the MESHERY_SERVER_CALLBACK_URL env var, the variable is not set inside the container.
  2. When using port-forward flag, a random port is chosen on the host device, and traffic is then directed to the server within the Pod. This presents challenges in specifying a custom value for MESHERY_SERVER_CALLBACK_URL, as this value must be provided at server startup. The PR ensures that whatever value is assigned to the environment variable, the port-forward will correctly forward to the corresponding value of the environment variable <host:port>.

Signed commits

  • Yes, I signed my commits.

Copy link

github-actions bot commented May 15, 2024

@MUzairS15 MUzairS15 requested a review from leecalcote May 15, 2024 10:59
Signed-off-by: MUzairS15 <[email protected]>
Signed-off-by: MUzairS15 <[email protected]>
)

// dashboardOptions holds values for command line flags that apply to the dashboard
// command.
type dashboardOptions struct {
host string
port int
podPort int
Copy link
Member

Choose a reason for hiding this comment

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

Or is this just the Kubernetes api address, not necessarily the specific host?

@@ -71,6 +72,9 @@ mesheryctl system dashboard
// Open Meshery UI in browser and use port-forwarding (if default port is taken already)
mesheryctl system dashboard --port-forward

Copy link
Member

Choose a reason for hiding this comment

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

Please clarify whether this port number assignment is by default or just an example. Why not use 9081 as the default (and as the example)?

Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

Will you confirm that the port-forward command gracefully handles platform != “kubernetes@ and informs the user that this is a platform-specific command?

@MUzairS15
Copy link
Contributor Author

MUzairS15 commented May 17, 2024

When used with docker, it just skips the protforward logic and directly opens the browser at endpoint as specified in mesheryctl context.
Screenshot 2024-05-17 at 8 50 36 PM

// @Jougan-0 You can quickly update, to add a warning message stating the flag is for when meshery is deployed on kubernetes platform before continuing to open the browser.

@@ -44,7 +44,7 @@ func NewPortForward(
ctx context.Context,
client *meshkitkube.Client,
namespace, deployName string,
host string, localPort, remotePort int,
host string, localPort, remotePort int, // I think "remotePort" should be idenitified dynamically based on the retrieved Pod configuration, instead of assuming 8080, or asking from user?
Copy link
Member

Choose a reason for hiding this comment

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

Yes, although, for the service to be listening on a port other than 8080, the user will have to be running custom Meshery image builds.

@leecalcote leecalcote merged commit 0b09cb7 into meshery:master May 17, 2024
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants