Skip to content

Fix UID Windows#175

Merged
danielsuguimoto merged 5 commits intokool-dev:masterfrom
fbrissi:master
Oct 23, 2020
Merged

Fix UID Windows#175
danielsuguimoto merged 5 commits intokool-dev:masterfrom
fbrissi:master

Conversation

@fbrissi
Copy link
Copy Markdown
Contributor

@fbrissi fbrissi commented Oct 22, 2020

Correction of generating uid for Windows and installing does not add kool on Windows startup.

  • As in Windows we don't have uid, use sid and get the last number, example: S-1-5-21-3623811015-3361044348-30300820-1013
    So, in this case, the uid used is 1013, this solve the problem to use root, but it still has problems as in other cases that we saw in linux in docker images
  • Fix the installer was putting the kool to boot with windows start
  • Add arguments to build_artifacts, sample: ./build_artifacts.sh --version 1.6.0

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 22, 2020

Codecov Report

Merging #175 into master will increase coverage by 2.74%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   86.62%   89.37%   +2.74%     
==========================================
  Files          49       52       +3     
  Lines        1563     1609      +46     
==========================================
+ Hits         1354     1438      +84     
+ Misses        190      145      -45     
- Partials       19       26       +7     
Impacted Files Coverage Δ
environment/asuser.go 100.00% <100.00%> (ø)
environment/env.go 80.64% <100.00%> (-0.61%) ⬇️
environment/uid.go 100.00% <100.00%> (ø)
cmd/exec.go 100.00% <0.00%> (ø)
cmd/root.go 100.00% <0.00%> (ø)
cmd/docker.go 100.00% <0.00%> (ø)
cmd/self-update.go 100.00% <0.00%> (ø)
cmd/kool_service.go 100.00% <0.00%> (ø)
cmd/fake_kool_service.go 100.00% <0.00%> (ø)
cmd/shell/output_writer.go 100.00% <0.00%> (ø)
... and 6 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 473bb70...2c2a23c. Read the comment docs.

Copy link
Copy Markdown
Contributor

@danielsuguimoto danielsuguimoto left a comment

Choose a reason for hiding this comment

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

Can't we merge asuser and uid files? They are already sharing the uid() function, so keeping both in a single file could be more readable, IMO. But let's check the other's opinions

Comment thread environment/uid_windows.go Outdated
Copy link
Copy Markdown
Member

@fabriciojs fabriciojs left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for improving the Windows experience!

Just one note about resilience to avoid errors on unexpected environments.

Comment thread environment/uid_windows.go Outdated
for i, name := range match {
results[sidExp.SubexpNames()[i]] = name
}
return results["uid"]
Copy link
Copy Markdown
Member

@fabriciojs fabriciojs Oct 22, 2020

Choose a reason for hiding this comment

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

what is the chance we run this in a different version of windows, and for some reason we do not get a proper "uid" match from FindStringSubmatch above?

I guess we should here check IF the "uid" key exists, and then return its value; other wise I guess we should not break but instead we can default to the old behaviour?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did it.
I tried to do a unit test to get this error, but I couldn't, I commented where the test failed.
I wanted to simulate a case of divine work. 😄

@fabriciojs
Copy link
Copy Markdown
Member

This is related to #42

@fabriciojs
Copy link
Copy Markdown
Member

@danielsuguimoto you can review and then merge!

Copy link
Copy Markdown
Contributor

@danielsuguimoto danielsuguimoto left a comment

Choose a reason for hiding this comment

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

LGTM

@danielsuguimoto danielsuguimoto merged commit 10d1c2c into kool-dev:master Oct 23, 2020
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