theia-ide / theia-apps

Theia applications examples - docker images, desktop apps, packagings
Apache License 2.0
1.04k stars 345 forks source link

[cpp,go,python,full] Sync single language images with full image if possible and update versions #372

Closed chrbayer closed 4 years ago

chrbayer commented 4 years ago

Where possible sync the single language images and the full images and the update versions of development tools in full and other images when in sync:

marcdumais-work commented 4 years ago

Thanks for this PR, @chrbayer - This looks like a nice update!

marcdumais-work commented 4 years ago

Thanks @chrbayer - I just did another pass and found just one minor thing. I need to test a bit, but so far the changes look good! I'll let you know how it goes with the tests,

chrbayer commented 4 years ago

@marcdumais-work Thank you very much for reviewing and testing. Please tell me if all tests and reviews are finished, so that I can include all findings in the PR. Should I make a sperate commit or just re-push fixed?

marcdumais-work commented 4 years ago

Should I make a sperate commit or just re-push fixed?

It's fine to force-push your fix(es).

Normally we prefer squashing the commits of PRs but in this case, looking a the commit messages, the breakdown looks logical and if you think you've maintained the changes pretty well insulated, each in their own commit, I have no problem retaining the various commits you have in the PR currently.

Though if we conclude it will make no difference in image size, we could return to a single cleanup at the end for apt temp files (i.e. drop last commit)

marcdumais-work commented 4 years ago

This PR introduces lots of changes, so it will be hard to test everything. I've done some sanity testing, just to confirm that the main things seem to still work. No issue found!

[full]:

[cpp]

[python]

marcdumais-work commented 4 years ago

@chrbayer Ready for your final update.

Can you let me know if anything substantial changes vs the current version, that would make it worth it to redo some of the testing?

chrbayer commented 4 years ago

@marcdumais-work Thank you very much for all you support! The only changes I did are the two proposed changes:

  1. Remove the commented line at the end of the cpp and full docker files
  2. Removing the apt cleanup after each step and do the cleanup only at the end of the script
chrbayer commented 4 years ago

For me this PR should now be complete :-)