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

COS-230 - Update the README for Windows users #34

Merged
merged 2 commits into from
Aug 15, 2022

Conversation

ioah86
Copy link
Contributor

@ioah86 ioah86 commented Aug 9, 2022

No description provided.

@carehart
Copy link

carehart commented Aug 9, 2022

A couple of thoughts: first, I'm surprised to hear that this issue (on Windows) is about symlinks. Again, since running the tool against a simple hello-world worked, I thought it had something to do with the size of the image (anyone following this who has not seen the related discussion @ioah86 linked to above may want to read that for context).

Second, I'm surprised also that you've listed Windows Dev mode as the first solution. That's the most onerous/wide-ranging/long-lasting of the 3 options. Really, I'd think the second should be listed first, as it's easiest. And while you've talked about how to do it with powershell, I'll note that one need not use that. I had shared how it worked from the simple command prompt, as long as that was opened "with admin". I'll understand if you prefer not to detail that, but you could at least say "open the Windows command prompt or powershell as admin", and then you could go on to detail only the latter.

Also, in that current point 2, you say "This Can be done..." with a capital C that you'll want to lowercase. :-)

Finally, your current point 3 discusses installing WSL. What I had shared originally was that I encountered this while running Docker via Docker Desktop for Windows (which is very commonly used for Windows folks), and that already implements WSL in latest versions. My suggestion was to point people to how they could implement python and coguard within the WSL vm they'd already have. Your current wording leaves the impression they have to install WSL. I suppose we can presume people who understand things can "figure it out".

Again, I leave all this simply as thoughts for you to consider. If you opt to weave those thoughts into any improvement, that's great. If you may prefer I offer a revision instead, I could do that. But I didn't want to just do that, as you may not agree with my contentions above. I thought it better to discuss them first. Totally your call how we proceed with any revisions, if at all.

As always, I'm just trying to help.

@ioah86
Copy link
Contributor Author

ioah86 commented Aug 9, 2022

Those are fair points. Let me see how I can address them. I will update the PR shortly

@ioah86
Copy link
Contributor Author

ioah86 commented Aug 9, 2022

But, just to give you the reason why I put the developer mode as first option: Coming from a Linux world, running something as administrator is an option that is usually exercised last. But I guess in Windows it is accepted as an in-betweener more readily. I will take your view for now, as I do think you know the Windows-user world better than I do.

@ioah86
Copy link
Contributor Author

ioah86 commented Aug 9, 2022

And re symlinks: python/cpython#95763 Apparently, that is something Windows has released later, but yes, it has something to do with Symlinks. And unfortunately, it is important in this software product: Take e.g. the mariadb docker image. The path to the actual configuration file is going through 2 symlinks.

@ioah86
Copy link
Contributor Author

ioah86 commented Aug 9, 2022

And, last but not least: Thank you for your help.

@ioah86
Copy link
Contributor Author

ioah86 commented Aug 9, 2022

Updated the PR. Please let me know if I missed anything @carehart

@ioah86 ioah86 merged commit 6d83871 into master Aug 15, 2022
@ioah86 ioah86 deleted the COS-230-update-readme-for-windows branch August 15, 2022 17:57
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.

2 participants