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.

Sign up for GitHub

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

Jump to bottom

use ide-proxy provide ide logo #7227

Merged
merged 1 commit into from
Jan 11, 2022
Merged

use ide-proxy provide ide logo #7227

merged 1 commit into from
Jan 11, 2022

Conversation

iQQBot
Copy link
Contributor

@iQQBot iQQBot commented Dec 14, 2021 •
edited
Loading

Description

use ide-proxy provide ide logo

Related Issue(s)

Fixes #6955

How to test

Release Notes

NONE

Documentation

@codecov
Copy link

codecov bot commented Dec 14, 2021 •
edited
Loading

Codecov Report

Merging #7227 (fd2e976) into main (3f730b1) will decrease coverage by 0.79%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##            main   #7227      +/-   ##
========================================
- Coverage   8.38%   7.58%   -0.80%     
========================================
  Files         33      31       -2     
  Lines       2410    2242     -168     
========================================
- Hits         202     170      -32     
+ Misses      2204    2070     -134     
+ Partials       4       2       -2
Flag Coverage Δ
components-gitpod-cli-app 9.53% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
installer-raw-app 5.76% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/local-app/pkg/auth/pkce.go
components/local-app/pkg/auth/auth.go

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 3f730b1...fd2e976. Read the comment docs.

@akosyakov
Copy link
Member

We need to think how we are going to deploy these changes, since they affect @gitpod-io/engineering-meta components.

@iQQBot
Copy link
Contributor Author

iQQBot commented Dec 14, 2021 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-pd-ide-proxy.2

@iQQBot
Copy link
Contributor Author

iQQBot commented Dec 14, 2021 •
edited
Loading

Only works for helm
installer need wait #7200 merged or split to other PRs

image

@iQQBot iQQBot force-pushed the pd/ide-proxy branch 2 times, most recently from e315774 to 692ea7b Compare December 15, 2021 03:54
@iQQBot
Copy link
Contributor Author

iQQBot commented Dec 15, 2021

ready for review

@JanKoehnlein
Copy link
Contributor

/lgtm
/hold

Please unhold when the order of the deployment is clear. Look like we should deploy IDE first, then meta.

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

LGTM label has been added.

Git tree hash: 659fb5f5acc0d48f16f2efc68c5ba1e6a9cbb672

@akosyakov
Copy link
Member

I think it looks good, now we need to wait till the ide proxy is deployed.

Please unhold when the order of the deployment is clear. Look like we should deploy IDE first, then meta.

Yes, I think we need to think about backward compatibility here:

  • if IDE deploys first it seems to be alright, since proxy will be there
  • if meta deploys first it will break. @iQQBot do you think extracting dashboard changes to another PR will help?

@iQQBot
Copy link
Contributor Author

iQQBot commented Dec 15, 2021

Just deploy the ide-proxy on prod first and test it OK, then merge this PR and wait for the meta to be deployed.

@akosyakov
Copy link
Member

merge this PR and wait for the meta to be deployed.

That's not necessary going to work, we have to deploy server configs regularly to bring new supervisor or some IDEs.
Let's wait a bit, but maybe we need to split this PR as well.

@iQQBot
Copy link
Contributor Author

iQQBot commented Dec 16, 2021

merge this PR and wait for the meta to be deployed.

That's not necessary going to work, we have to deploy server configs regularly to bring new supervisor or some IDEs. Let's wait a bit, but maybe we need to split this PR as well.

OK

@stale
Copy link

stale bot commented Dec 26, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Dec 26, 2021
@iQQBot iQQBot added meta: never-stale This issue can never become stale and removed meta: stale This issue/PR is stale and will be closed soon labels Dec 27, 2021
@JanKoehnlein
Copy link
Contributor

Is anybody still working on this? If not, I'd close it or at least unassign team meta

@iQQBot
Copy link
Contributor Author

iQQBot commented Jan 10, 2022 •
edited
Loading

Currently waiting platform team apply terraform to production once this done, we can merge this PR

@iQQBot
Copy link
Contributor Author

iQQBot commented Jan 10, 2022

I will unassign meta team , if platform team is apply terraform I will assign again

@iQQBot iQQBot removed the team: webapp Issue belongs to the WebApp team label Jan 10, 2022
akosyakov
akosyakov approved these changes Jan 11, 2022
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

remove hold if you synced with meta deployment and sure that merging and deploying it now does not break production

@roboquat roboquat added team: webapp Issue belongs to the WebApp team and removed lgtm labels Jan 11, 2022
akosyakov
akosyakov approved these changes Jan 11, 2022
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

/lgtm

@roboquat roboquat added the lgtm label Jan 11, 2022
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 9bcaacc1b03e014f678aac27622932823ca49632

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akosyakov, JanKoehnlein

Associated issue: #6955

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

@iQQBot
Copy link
Contributor Author

iQQBot commented Jan 11, 2022

/unhold

@roboquat roboquat merged commit 4bdcf8c into main Jan 11, 2022
@roboquat roboquat deleted the pd/ide-proxy branch January 11, 2022 13:18
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov approved these changes

Assignees

@JanKoehnlein JanKoehnlein

@akosyakov akosyakov

Labels
approved deployed: webapp Meta team change is running in production meta: never-stale This issue can never become stale release-note-none size/L team: IDE team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ide] Serve IDE logos independently of the dashboard
4 participants
@iQQBot @akosyakov @JanKoehnlein @roboquat