eclipse-che / che-theia

Eclipse Public License 2.0
124 stars 110 forks source link

chore: update local git config when user changes Che Theia git settings #1319

Closed vinokurig closed 2 years ago

vinokurig commented 2 years ago

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

https://github.com/eclipse/che/issues/21115

How to test this PR?

run factory:

https://github.com/che-samples/bash/tree/devfilev2?che-editor=https://gist.githubusercontent.com/vinokurig/8240d845067c89e295e2cb4dc2c0070b/raw/ad3452ee49b0eeddfaf086f55557d46206073fea/che-theia.yaml
  1. Set some gitconfig settings e.g. user.name and user.email.
  2. Restart workspace.

See: The gitconfig settings are present.

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Happy Path Channel

HAPPY_PATH_CHANNEL=stable

vinokurig commented 2 years ago

depends on https://github.com/eclipse/che/issues/21147

codecov[bot] commented 2 years ago

Codecov Report

Merging #1319 (c1aab10) into main (c299f59) will increase coverage by 3.95%. The diff coverage is 39.52%.

@@            Coverage Diff             @@
##             main    #1319      +/-   ##
==========================================
+ Coverage   32.78%   36.74%   +3.95%     
==========================================
  Files         290      330      +40     
  Lines        9885    11393    +1508     
  Branches     1457     1572     +115     
==========================================
+ Hits         3241     4186     +945     
- Misses       6641     7202     +561     
- Partials        3        5       +2     
Impacted Files Coverage Δ
...theia-about/src/browser/about-che-theia-dialog.tsx 0.00% <0.00%> (ø)
...credentials/src/browser/che-credentials-service.ts 0.00% <0.00%> (ø)
...entials/src/browser/credentials-frontend-module.ts 0.00% <0.00%> (ø)
...eia-credentials/src/common/credentials-protocol.ts 0.00% <0.00%> (ø)
...eia-credentials/src/node/che-credentials-server.ts 0.00% <0.00%> (ø)
...s/src/node/che-theia-credentials-backend-module.ts 0.00% <0.00%> (ø)
...ashboard/src/browser/che-theia-dashboard-module.ts 0.00% <0.00%> (ø)
...ia-dashboard/src/browser/theia-dashboard-client.ts 0.00% <0.00%> (ø)
...rowser/src/browser/che-mini-browser-environment.ts 0.00% <0.00%> (ø)
...in-ext/src/browser/che-sidecar-file-system-main.ts 100.00% <ø> (ø)
... and 295 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 c58f5bd...c1aab10. Read the comment docs.

l0rd commented 2 years ago

I have just started a workspace using this editor definition, and although the git user and email are set in Theia preferences, Theia still shows the warning in the status bar:

image

Note that theia git email and username preferences were set previously, in another workspace.

l0rd commented 2 years ago

And git commit from the terminal doesn't seem to like:

image
l0rd commented 2 years ago

After editing the preferences the warning in the status bar disappear. Even when I restart the workspace 👍 But the terminal is still unhappy.

image

vinokurig commented 2 years ago

@l0rd I've fixed all the issues above, could you please take a look?

l0rd commented 2 years ago

Thanks @vinokurig I will test it later

l0rd commented 2 years ago

I have tried this PR and there is a problem. Theia now creates a configmap with the gitconfig. But if a gitconfig ConfigMap already existed in the namespace we will end up with 2 ConfigMaps:

$ oc get cm -l controller.devfile.io/mount-to-devworkspace=true
NAME                  DATA   AGE
git-config            1      12d
workspace-gitconfig   1      74m

And then workspaces will fail to start.

l0rd commented 2 years ago

As discussed a couple of weeks ago I think that Theia should not create or update the git configmap. It should instead:

l0rd commented 2 years ago

Other acceptance criteria:

vinokurig commented 2 years ago

PR update:

@l0rd @svor Please take a look

vinokurig commented 2 years ago

To test this PR run a factory:


https://github.com/che-samples/bash/tree/devfilev2?che-editor=https://gist.githubusercontent.com/vinokurig/8240d845067c89e295e2cb4dc2c0070b/raw/ad3452ee49b0eeddfaf086f55557d46206073fea/che-theia.yaml
l0rd commented 2 years ago

I have tried your Theia image as mentioned in this PR description. I don't understand why I am asked to configure my git config email when it's already configured:

image

Note that I had a theia preferences configmap:

kubectl cm workspace-preferences-configmap -o json | jq -r '.data[]' | jq .
{
  "git.user.email": "mario.loriedo@gmail.com",
  "git.user.name": "Mario"
}
vinokurig commented 2 years ago

@l0rd @svor

I have tried your Theia image as mentioned in this PR description. I don't understand why I am asked to configure my git config email when it's already configured

I've fixed this bug, could you please try again?

vinokurig commented 2 years ago

@l0rd @svor There is another bug: panel is not updated if a new workspace started with existing user git preferences, working on it

vinokurig commented 2 years ago

@l0rd @svor

There is another bug: panel is not updated if a new workspace started with existing user git preferences, working on it

The bug has been fixed, could you please take a look?

svor commented 2 years ago

seems ok to me, thanks for updating the PR

svor commented 2 years ago

@l0rd @azatsarynnyy we are going to merge this PR after 2 days. If you have any concerns with that, please let us know