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

[3007.x] fix regression in master publish ipc socket #66752

Closed
wants to merge 3 commits into from

Conversation

Sxderp
Copy link

@Sxderp Sxderp commented Jul 26, 2024

Fixes #66228

Though the sockets /may/ need to be deleted or permission changed if they already exist. Not sure if the code will change the permissions when websocket / tcp transport is selected.

@Sxderp Sxderp requested a review from a team as a code owner July 26, 2024 19:04
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title fix regression in master publish ipc socket [3007.x] fix regression in master publish ipc socket Jul 26, 2024
@dmurphy18 dmurphy18 requested a review from dwoz July 26, 2024 19:10
@dwoz dwoz added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jul 28, 2024
@Sxderp
Copy link
Author

Sxderp commented Jul 30, 2024

I see the needs testcase has been added. I'll try and figure that out. I'm not confident in my understanding of the test suite. I think that I'll need a test case for each transport to ensure that each one creates a permissive socket. Am I on the right track?

If you could point me to some similar tests that would be helpful.

I'm also not sure how to ensure that the socket doesn't already exist due to previous tests. I guess I just delete the file before the start of the tests?

@Sxderp
Copy link
Author

Sxderp commented Jul 30, 2024

I believe I have some working tests. A few things to point out.

  1. These's a lot of duplicated code. I don't know how to minimize this in a test environment.
  2. There are no websocket tests. I created some but websockets are broken and need additional code to actually work. I might be able to submit a PR for that, but there's some oddities with websockets. Like async functions that never get awaited / finished. I find this confusing because both tornado and asyncio are being used.

@Sxderp
Copy link
Author

Sxderp commented Aug 14, 2024

Closing in favor of #66805

@Sxderp Sxderp closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants