Skip to content

Conversation

trumbitta
Copy link
Contributor

@trumbitta trumbitta commented Mar 28, 2022

Description

Set 2 variables once instead of editing craco.config.js every time you want to contribute to the dashboard app.

How to test

  • Follow the new instructions in components/dashboard/README.md
  • Check if the dashboard loads properly
  • 🚨 IMPORTANT! Do a production build and check that one as well: I never used CRACO's whenDev before

Release Notes

[dashboard] It's now even easier to contribute: set a couple variables once, contribute for as long as you like.

@trumbitta
Copy link
Contributor Author

trumbitta commented Apr 5, 2022

🆙 I'm quite fond of this one, so 🤞

@laushinka
Copy link
Contributor

/werft run

1 similar comment
@laushinka
Copy link
Contributor

/werft run

@laushinka
Copy link
Contributor

Have to run again due to Werft issues earlier.

/werft run

@laushinka
Copy link
Contributor

Again 😭 [internal Slack]

/werft run

},
},
...whenDev(() => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

devServer is always using in dev, you can add some check for process.env.GP_DEV_HOST and process.env.GP_DEV_COOKIE to make sure someone want to enable this feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this necessary? The instructions before this change were like: if you want to contribute to the dashboard app, you have to add a devServer block with this and that.
The changes in this PR just make those instructions easier: you don't have to add that block every time you open a workspace, and you don't have to make sure every time that you don't commit and push that block.

Are there more, undocumented, situations where one doesn't need to add a devServer block in order to work on the dashboard app?

Copy link
Contributor

Choose a reason for hiding this comment

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

member can use telepresence for development, it need permission for preview environment kubectl

Copy link
Contributor

Choose a reason for hiding this comment

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

@iQQBot Will we hit this whenDev() block when we use telepresence?

Copy link
Contributor

Choose a reason for hiding this comment

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

image
I think it will hit, because telepresence also use craco start

Copy link
Contributor Author

@trumbitta trumbitta Apr 7, 2022

Choose a reason for hiding this comment

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

@mustard-mh The exact same reason why before this we had to add the whole devServer block to the config instead of just its contents: I don't know 🤣

To me, it's also more explicitly saying: "Ok, this one is only for development" vs having to know how Webpack handles this specific config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have a notion document about using telepresence, current is internal, But to be honest, I didn't use it 😂

image I do some test, when someone use telepresence is will in whenDev

@iQQBot @laushinka (I'm still going to act on your initial feedback ASAP, but I'd like to understand) If it's correct for telepresence to run in dev mode, then why isn't it correct for telepresence to also use the devServer?

Copy link
Contributor

Choose a reason for hiding this comment

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

For telepresence, which works by replacing pods in a kubernetes cluster and redirecting traffic to access them, it has strong limitations, for example, it can only debug the current preview environment, it cannot debug gitpod.io

The reason I'm using devServer here is that our server has access restrictions on origin, and ws-proxy automatically filters out sensitive cookies, making it impossible to skip these restrictions at the browser level without using devServer's forwarding capabilities

Copy link
Contributor

Choose a reason for hiding this comment

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

Another reason I wanted to add the check is that if a user doesn't have GP_DEV_HOST configured then bringing in devServer results in all his requests being directed to https:// which is an address that can never be reached and can be confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @iQQBot for jumping in and thanks @trumbitta for this implementation! This change will greatly benefit our contributors 🙏🏽
The use of telepresence is explained in an internal doc, but it's a good point for us to reconsider moving it closer to our code.
And for now as @iQQBot explained, for this change, we should make sure that only contributors will hit the devServer block by explicitly checking for the env variables.
Looking forward to that change and shipping this 🔥

@geropl geropl added the team: webapp Issue belongs to the WebApp team label Apr 7, 2022
@mustard-mh
Copy link
Contributor

mustard-mh commented Apr 7, 2022

Good idea🤩 ,

I used to add const COOKIE = require('fs').readFileSync(".dev.local").toString() and apply it before 😂

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.

I would like with this

@trumbitta trumbitta force-pushed the streamline-dashboard-dev-env branch from 76817b4 to 0c56543 Compare April 7, 2022 15:21
@trumbitta
Copy link
Contributor Author

trumbitta commented Apr 7, 2022

@laushinka I followed the example by @iQQBot and updated the code 🧡

@laushinka
Copy link
Contributor

laushinka commented Apr 8, 2022

Tested and works as advertised!
Thank you again @trumbitta 🤗 and apologies for the delay in reviewing 🙏🏽

LGTM

@laushinka
Copy link
Contributor

laushinka commented Apr 8, 2022

Seems like I have to do this again 🤔

/werft run

👍 started the job as gitpod-build-streamline-dashboard-dev-env-fork.4

@mustard-mh
Copy link
Contributor

mustard-mh commented Apr 8, 2022

/werft run no-preview

👍 started the job as gitpod-build-streamline-dashboard-dev-env-fork.5

@roboquat roboquat merged commit 06bd515 into gitpod-io:main Apr 8, 2022
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta 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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants