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

[installer]: separate server and IDE components #7200

Merged
merged 3 commits into from
Dec 14, 2021

Conversation

mrsimonemms
Copy link
Contributor

Description

As the server component is currently structured, it doesn't accurately reflect the team ownership. The server component is owned by @gitpod-io/engineering-meta and the ide-configmap is owned by the @gitpod-io/engineering-ide team.

This separates out the ide-configmap into it's own package underneath server with the corrected ownership

Related Issue(s)

Fixes #7060

How to test

Run a deployment via the Installer as normal. There are no changes to the way the Installer behaves, just code structure changes

Release Notes

[installer]: separate server and IDE components

Documentation

@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #7200 (e5fb617) into main (300a805) will not change coverage.
The diff coverage is n/a.

❗ Current head e5fb617 differs from pull request most recent head a2d9783. Consider uploading reports for the commit a2d9783 to get more accurate results
Impacted file tree graph

@@          Coverage Diff          @@
##            main   #7200   +/-   ##
=====================================
  Coverage   5.76%   5.76%           
=====================================
  Files         13      13           
  Lines       1162    1162           
=====================================
  Hits          67      67           
  Misses      1094    1094           
  Partials       1       1           
Flag Coverage Δ
installer-raw-app 5.76% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 ced67ae...a2d9783. Read the comment docs.

@mrsimonemms mrsimonemms requested review from a team December 13, 2021 12:02
@mrsimonemms
Copy link
Contributor Author

/hold

I would like to get approval from all three teams in the approvals as it affects them all

@akosyakov
Copy link
Member

akosyakov commented Dec 13, 2021

@iQQBot Could you review it please? 🙏

@iQQBot
Copy link
Contributor

iQQBot commented Dec 13, 2021

@iQQBot Could you review it please? 🙏

Yes

@JanKoehnlein
Copy link
Contributor

/lgtm

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: a24815ca83db8bdbcfbb09dfdb9344bc602d0525

Copy link
Contributor

@iQQBot iQQBot left a comment

Choose a reason for hiding this comment

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

LGTM, just change path, no changes to other cores
@akosyakov I am not owner of IDE team

@mrsimonemms
Copy link
Contributor Author

Only question remains - should there be multiple ownerships on the versions folder?. For this, it makes sense to me as this is the one part that really is owned by everyone.

But equally, I can see the argument that this should be owned by the owner of the component as the custodians/maintainers of the Installer.

Thoughts @svenefftinge @csweichel @corneliusludmann et al

@corneliusludmann
Copy link
Contributor

Only question remains - should there be multiple ownerships on the versions folder?. For this, it makes sense to me as this is the one part that really is owned by everyone.

Ideally, we would find a way to split this file and have a separate versions.go file for each team. However, this seems to involve a lot of effort, doesn't it? That's why I think it's a feasible compromise that all teams are owners of this file.

@mrsimonemms
Copy link
Contributor Author

@corneliusludmann yes, it would probably require reworking of the whole version generator in werft, which could well have unforeseen consequences. It also seems a lot of work when could just have multiple owners in there

@corneliusludmann
Copy link
Contributor

corneliusludmann commented Dec 13, 2021

it would probably require reworking of the whole version generator in werft

I think it's not that worse. We “just” need to change the values of meta-data.helm-component in the BUILD.yaml files of all the components. E.g. change

metadata:
helm-component: server

to helm-component: meta.server and

metadata:
helm-component: workspace.desktopIdeImages.intellij

to helm-component: ide.desktopIdeImages.intellij.

Then, everything below meta in the versions.yaml would go to the meta team and everything below ide team would go to the IDE team etc.

However, this change does not make sense as long as we support the helm charts because that would break them. But we could think about making this change once we stop supporting the helm charts.

For now, multiple owners of the versions.go file would be fine, in my opinion.

@iQQBot
Copy link
Contributor

iQQBot commented Dec 13, 2021

Hi @mrsimonemms I create a PR for cleanup deprecated ide-config, if you approve it will merge into this branch
#7212

@roboquat roboquat removed the lgtm label Dec 13, 2021
@iQQBot iQQBot self-requested a review December 13, 2021 15:35
Copy link
Contributor

@iQQBot iQQBot left a comment

Choose a reason for hiding this comment

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

LTGM

@roboquat roboquat added the lgtm label Dec 13, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 4fff918a745a846abc396f70cdd0e17551da8306

@jeanp413
Copy link
Member

@iQQBot I think you should also cleanup chart/templates/server-ide-configmap.yaml 👀

@iQQBot
Copy link
Contributor

iQQBot commented Dec 13, 2021

@iQQBot I think you should also cleanup chart/templates/server-ide-configmap.yaml 👀

#7211 already done in another PR

@mrsimonemms
Copy link
Contributor Author

@jeanp413 that shouldn't be part of this PR though

@iQQBot
Copy link
Contributor

iQQBot commented Dec 14, 2021

/verify-owners

@iQQBot
Copy link
Contributor

iQQBot commented Dec 14, 2021

I think this PR should be merged as soon as possible

@mrsimonemms mrsimonemms requested a review from a team December 14, 2021 20:36
@mrsimonemms
Copy link
Contributor Author

/unhold
/approve

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iQQBot, JanKoehnlein, MrSimonEmms

Associated issue: #7060

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 5142a93 into main Dec 14, 2021
@roboquat roboquat deleted the sje/installer-ide-components branch December 14, 2021 20:40
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note size/L team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix ownership of installer files that belong to team IDE
7 participants