Extract waitForGvProxy into shared utility function #21694
Conversation
arixmkii
force-pushed
the
wait-for-gvproxy
branch
from
c5d50d6 to
4669525
Compare
|
Fixed now. |
arixmkii
force-pushed
the
wait-for-gvproxy
branch
from
4669525 to
198986d
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
| backoffWait := backoff | ||
| logrus.Debugf("checking that %q is ready", name) | ||
| for i := 0; i < maxBackoffs; i++ { | ||
| _, err := os.Stat(socketPath) |
There was a problem hiding this comment.
Why split this out? There is really no big reason to use unix.Access() over os.Stat() here so I rather have one function for everything. The duplication is always incredibly hard to work with.
There was a problem hiding this comment.
I can use os.Stat version for all platforms. I didn't want to change the original implementation, my assumption was that the unix.Access with syscall inside was an intentional optimization (I know this is not a hot path, so, it should not be as critical). I also saw that in some other projects there was a need to use os.Lstat for checking socket on Windows https://github.com/lima-vm/lima/blob/e9105bd92211bc25029d2c4b87dff404e043f65e/pkg/httpclientutil/httpclientutil_windows.go#L14, but I could not reproduce the error on recent Windows builds using latest golang. I can't say if this will have to be addressed additionally or not in the future.
If using os.Stat for all platforms will be accepted I will change into unified version.
There was a problem hiding this comment.
im ok with where this is going ... the timing is bad, but generally this is the direction we were headed. I would prefer paths remain typed as vmfiles.
in the future, i want a vmsocket type, so that we can check socket len(apple) and do various forms of open, listen, etc. but i had envisioned more of an interface type than by OS.
lets review this with scrutiny given we are nearing a release and if it looks like any form of improvement, then im ok to merge.
There was a problem hiding this comment.
I would prefer paths remain typed as vmfiles.
sockets.go was using VMFile only as an output parameter, I believed this as a design choice and that was the reason to change to plain paths. I can return VMFile, if this is desired.
in the future, i want a vmsocket type
Then if this or similar change would be introduced it will be easier to find everything what is desired to be changed in one place (or close to each other at least).
|
Moving to an OS-agnostic implementation SGTM |
Signed-off-by: Arthur Sengileyev <[email protected]>
arixmkii
force-pushed
the
wait-for-gvproxy
branch
from
198986d to
49400ec
Compare
|
Changed to a single OS agnostic version. Manually checked on M1 macOS: happy case and debug logging verified. |
|
LGTM |
|
LGTM |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arixmkii, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
stale-locking-app
bot
added
the
locked - please file new issue/PR
Making
waitForGvProxyavailable for use in other providers as it has noapplehvspecifics. New location would be withinsocketspackage, where other socket related utilities could be found.This could be then used with other providers like proposed change here #21594 where right now there is need to duplicate this code.
Does this PR introduce a user-facing change?
[NO NEW TESTS NEEDED]