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

Update xpk.py #92

Closed
wants to merge 1 commit into from
Closed

Conversation

sadikneipp
Copy link
Collaborator

Remove server argument from proxy server arguments.

Fixes / Features

  • Without this change, the proxy server was not starting up for me. I'd get an unknown argument error.

Testing / Documentation

This unblocked me from running a workload.

Remove server argument from proxy server
@sadikneipp sadikneipp requested a review from Obliviour as a code owner March 20, 2024 03:17
@Obliviour Obliviour requested a review from RoshaniN March 20, 2024 17:27
Copy link
Collaborator

@Obliviour Obliviour left a comment

Choose a reason for hiding this comment

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

Thanks @sadikneipp for the fast change!

  1. Nit to change the PR Name to be more specific.

  2. @RoshaniN can you confirm if this flag is needed for other aspects -- is this the right fix for the issue?

@RoshaniN
Copy link
Collaborator

Hey Sadi,
I am curious to know if any logic around this argument was recently changed in Pathways, as I did not face this issue.

@Obliviour
Copy link
Collaborator

Looking at the Build Test failure, you may need to create a PR from a branch instead of a fork. I provided you permissions to do this, just in case.

Copy link
Collaborator

@RoshaniN RoshaniN left a comment

Choose a reason for hiding this comment

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

Thanks for the change, Sadi!

Looks like the flag fails after adding cl/607802178, which will be merged soon. Currently this flag is not super useful either, so I see no issues removing it.

Changes are GTG once the branch is rebased onto main .

@sadikneipp
Copy link
Collaborator Author

Since it's such a small change @RoshaniN aligned with me to incorporate this PR in #100 . I'll close this PR.

@sadikneipp sadikneipp closed this Mar 27, 2024
@sadikneipp sadikneipp deleted the patch-1 branch April 21, 2024 05:07
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