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

ui: Pass through default environment settings when when booting dev/test #14519

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

johncowen
Copy link
Contributor

Description

Previous to this PR we had a way during dev or testing to enable/disable all sorts of feature flags via cookies, this PR gives those feature flags defaults based on what is in the corresponding Ember environment variables for the given environment. Note: This is only happens in non-production environments.

There are some tiny coincidental changes here due to HCPEnabled not having a default boolean value when not in production environments (undefined being falsey), in prod envs that boolean comes in from the backend.

This PR is required to move forwards with some additional peering testing, that will come in some follow up PRs

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

@johncowen johncowen added the theme/ui Anything related to the UI label Sep 8, 2022
@johncowen johncowen added the pr/no-changelog PR does not need a corresponding .changelog entry label Sep 8, 2022
Copy link
Contributor

@LevelbossMike LevelbossMike left a comment

Choose a reason for hiding this comment

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

We should probably investigate how to handle this in a less complicated way. It feels like the environment variable setup we have currently touches many layers and is complicated to work with. This apparently comes from the custom startup we have in place. In a common ember app setup, this would just work™ based on config/environment.js that would either exist at build time or would get injected at runtime before application boot from the backend.

Approving to unblock the acceptance test, but in my mind we should really investigate how we can improve this for the future.

@johncowen
Copy link
Contributor Author

johncowen commented Sep 12, 2022

Thanks for the review @LevelbossMike !

We should probably investigate how to handle this in a less complicated way.

Absolutely agree.

What is covered here:

  1. Allowing folks/engineers to set/hardcode default feature flags per environment.
  2. Allowing folks/engineers to set feature flags via bash environment variables for test/CI purposes.
  3. Allowing folks/engineers to set feature flags without having to restart the ember build process.
  4. Allowing folks/engineers to set feature flags that effect the mocked HTTP API during testing..
  5. Allowing the Consul binary in production set feature flags and in turn our users set feature flags using Consul operator configuration.

One of the files in the PR is a test and

this would just work™ based on config/environment.js that would either exist at build time or would get injected at runtime before application boot from the backend.

...is pretty much how it works now ^. So I think we are on the same page here. I would love to discuss with you how we could reduce and refactor this code slightly to make a more reusable package as a I think a lot of the above isn't available in Ember by default, also I think a few of our UIs have the same need. From what I understand I think we'll be writing this up fairly soon 🙌

Oh interestingly there is a very recent issue on the Ember RFC repo that touches on this:

I'd like to gauge interest / comments for an easier way to customize index.html output.
...it would be cleaner if something like this was available in core. Customizing index.html is pretty common.

emberjs/rfcs#845

Anyway lets get this merged and we can discuss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-changelog PR does not need a corresponding .changelog entry theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants