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

Older windows support branch #310

Merged

Conversation

g-prasanth
Copy link
Contributor

Current operating system check, will allow only windows 10, 11, server 2016 & 2019.
There are older versions supported OSs, which version number is not MINGW64_NT-10.0*. So, I removed version check and allowed to install in all versions windows OS.

I referred this url, to verify different kernal versions in windows.

@codecov
Copy link

codecov bot commented Jun 28, 2021

Codecov Report

Merging #310 (9400e2d) into master (95ae738) will increase coverage by 1.36%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #310      +/-   ##
==========================================
+ Coverage   69.74%   71.10%   +1.36%     
==========================================
  Files          27       31       +4     
  Lines        1094     1478     +384     
==========================================
+ Hits          763     1051     +288     
- Misses        298      385      +87     
- Partials       33       42       +9     
Impacted Files Coverage Δ
talismanrc/talismanrc.go 77.30% <0.00%> (-3.95%) ⬇️
detector/pattern/pattern_detector.go 96.96% <0.00%> (-1.31%) ⬇️
scanner/scanner.go 18.18% <0.00%> (-0.24%) ⬇️
utility/sha_256_hasher.go 100.00% <0.00%> (ø)
detector/pattern/match_pattern.go 100.00% <0.00%> (ø)
detector/filecontent/hex_detector.go 100.00% <0.00%> (ø)
detector/helpers/checksum_compare.go 100.00% <0.00%> (ø)
detector/filename/filename_detector.go 100.00% <0.00%> (ø)
detector/filesize/filesize_detector.go 100.00% <0.00%> (ø)
detector/filecontent/shannon_entropy.go 100.00% <0.00%> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95ae738...9400e2d. Read the comment docs.

@g-prasanth g-prasanth closed this Jun 28, 2021
@g-prasanth g-prasanth reopened this Jun 28, 2021
@svishwanath-tw
Copy link
Collaborator

@g-prasanth : The only thing worrying me is how to support bug reports on older versions of windows. We will need to be able to re-create bugs or practically look for work arounds on those versions.

@g-prasanth
Copy link
Contributor Author

The only supported Windows OSs which are not NT-10.0* are Windows 8 and 2012. And I've tested the talisman setup in both the OSs. I've tested in an unsupported windows version which is 2008R2. Hope most of the scenarios covered.

@harinee
Copy link
Collaborator

harinee commented Jun 29, 2021

This is great! Thanks @g-prasanth
When we had initially coded to support Win10, it was primarily because almost all our Windows users belonged to that version.
My suggestion is to put in a line in the documentation (including gh-pages) to call out the versions tested and supported. I share @vhasus 's concern with older Windows, given file systems in older Windows versions could give troubles we probably haven't foreseen yet. I would suggest that we should be transparent in also explaining that in the event of triaging bugs, we would prioritise later versions of Windows (i.e. 10) over older ones. Let me know if you folks think that's fair.

@svishwanath-tw
Copy link
Collaborator

@g-prasanth : please update the README.md to specify which versions of windows are supported/tested like @harinee suggested

@dcRUSTy
Copy link
Collaborator

dcRUSTy commented Jun 29, 2021

  1. The MINGW32_NT* check should be valid for all NT version starting with Windows NT 3.1 (Year 1993)
  2. The golang(1.10+) https://github.com/golang/go/issues/23380 compiled binaries i think are only compatible with >(=?)Windows Vista.
  3. Apart from the binary, talisman's install/update scripts will definitely get TLS version issues or SSL certificate issues some day on old OS's

@svishwanath-tw
Copy link
Collaborator

@dcRUSTy, @harinee :
If we drop support for 32 bit windows, XP will be the oldest OS talisman could run on.
SSL version incompatibility will rule out web based installation on XP machines.
That only leaves out windows 7.
What do you folks think about dropping support for 32 bit OSes ?

@dcRUSTy
Copy link
Collaborator

dcRUSTy commented Jul 2, 2021

May be lets add a line in Readme.md that talisman binary may be compatible with older version of windows(because of golang) and thats it....
Lets not change installation script....
If a user knows about git hooks it wont take more than a minute to figure out how to use talisman binary and how to remove if they are facing issues.

@svishwanath-tw
Copy link
Collaborator

@dcRUSTy : If the idea is to overlook installations on unsupported versions, I think we should change the scripts to be past and future proof (ie. accept this PR) and put in a disclaimer stating what versions we've successfully tested talisman on.

@dcRUSTy
Copy link
Collaborator

dcRUSTy commented Jul 5, 2021

ok 👍

@g-prasanth
Copy link
Contributor Author

Thanks everyone. If any user faces issue in unsupported windows version, we don't need to troubleshoot and fix it since the OS is not supported by windows itself. But our talisman binary will work in most of the currently being used OS versions.

I will commit the disclaimer along with the tested versions.

@g-prasanth
Copy link
Contributor Author

Sorry for the delay. I've added disclaimer in readme.

@svishwanath-tw svishwanath-tw merged commit 5d61beb into thoughtworks:master Jul 19, 2021
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.

None yet

4 participants