-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🪟 🔧 Remove demo mode from UI #15746
🪟 🔧 Remove demo mode from UI #15746
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a code-overview it looks fine. I'm not sure how I could test this locally.
Yeah, I'm not really sure how to test this locally either, since this is just removing all of the IS_DEMO stuff, which is only really relevant to the demo deployment. So it feels like all we can really do is wait until this is merged, and then deploy the demo application to see if it looks correct (which I expect it to since it should now be the same as non-demo). I suppose we could also deploy the demo app from this branch so we can test it out before merging, but I'm not exactly sure how that is done. @nataliekwong do you know how we currently deploy the demo instance, or do you know who would know about that? |
Yes! @marcosmarxm directed me to this page before on how to access it: https://www.notion.so/Demo-Airbyte-Instance-b2b8f1ddfa53438db78433cca0ae0568 |
Thank you for linking that, super helpful! @krishnaglick in that Notion page linked above, it says that the command to deploy airbyte in the demo instance is
So I tried running that locally on this PR branch, and I confirmed that the demo mode banner is no longer present in the UI, and the rest of the app seems to be working as expected. So I think that should be sufficient for local testing |
8a2d0ce
to
2558869
Compare
@lmossman this won't break the demo.airbyte.io stuff? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it's looking good. I found an issue that needs to be fixed.
charts/airbyte-webapp/values.yaml
Outdated
<<<<<<< HEAD | ||
|
||
secrets: {} | ||
======= | ||
>>>>>>> 8a2d0ce928 (remove demo mode from UI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unresolved conflict, whoops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Looks like I used the vs code "accept incoming change" button incorrectly somehow. Pushed the fix!
@marcosmarxm See #15648 (comment) for details. There is already a new way to set up the messaging that the instance is a demo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm marking this as approved, pending Edmundo's greenlight and that merge message removed 👍
@marcosmarxm as Natalie stated in her comment on the related issue here (#15648 (comment)), we are going to use Google Optimize to display the banner instead of this custom logic. However, I am actually not sure what causes the demo application to be read-only. When I tested locally on master (i.e. without these changes), setting EDIT: see comment below - I found where this is configured, so feel free to disregard this comment |
Aha, I found where we configured the demo app to fail non-read API calls: airbyte/terraform/aws/demo/lb/main.tf Lines 87 to 107 in 381326f
Since that is configured through terraform, that should not be affected by any of the changes in this PR. So this should be safe to merge! |
* remove demo mode from UI * remove merge conflict
What
Resolves #15648
How
Removes the demo mode banner from the UI, as well as any code related to that banner (e.g. env vars and intl messages), as we will now be using a third-party tool to display that banner on the demo airbyte deployment.
There is a separate PR to remove the env var from the one place it exists in the cloud repo as well: https://github.com/airbytehq/airbyte-cloud/pull/2397