-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: adding frontend and backend feature flags #817
feat: adding frontend and backend feature flags #817
Conversation
@@ -33,6 +33,15 @@ function isModuleEnabled(module) { | |||
return getModuleActiveStatus(module); | |||
} | |||
|
|||
function isAnyFeatureModuleEnabled() { |
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 a little unsure about this method.. We basically have no nice way to tell if a module is a feature module or not. If we want to avoid this hardcoded if.. We could add a flag into the config.json that a module is considered a feature module...
Or perhaps make a new component like EnvironmentFeatures.js but with Toggles? And loop through the array? Let me know what you prefer.
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.
To this issue we also have the challenge of managing dependencies. At the moment they are hardcoded in the isModuleEnabled
function. Maybe we can think of a generic way of defining some module properties in the frontend as we do in the __init__
files for backend. I spoke about this with Mo as one of the future improvements for frontend. For the moment i think it is good
@dlpzx @noah-paige can you take a look? thoughts on the comment? |
frontend/src/modules/Environments/components/EnvironmentTeams.js
Outdated
Show resolved
Hide resolved
I tested in an AWS Deployment with the feature flags both on and off and is working as expected One other thing I noted is that we use default Alternatively, we can make overview the default Table tab opened (i.e. cc: @dlpzx @zsaltys |
@@ -157,7 +163,7 @@ const TableView = () => { | |||
const client = useClient(); | |||
const navigate = useNavigate(); | |||
const [table, setTable] = useState({}); | |||
const [currentTab, setCurrentTab] = useState('preview'); | |||
const [currentTab, setCurrentTab] = useState(tabs[0].value); |
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.
@noah-paige Actually this was a mistake on my side... When contributing back Mohit's changes I forgot to pick this up from our branch. This will now always default to the first tab on the 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.
Deployed these changes to AWS and tested with all the feature flags enabled and disabled - all working as expected
Also tested disabling a module and it is removed from the Env Feature Management card appropriately
Thank you @zsaltys for this PR - I will merge this through to |
### Feature or Bugfix - Feature - Bugfix - Refactoring ### Detail #### Features * Limit pivot role S3 permissions by @dlpzx in #780 * Limit pivot role KMS permissions by @dlpzx in #830 * Add configurable session timeout to IDP by @manjulaK in #786 * Allow to submit a share when you are both an approver and a requester by @zsaltys in #793 * Redirect upon creating a share request by @zsaltys in #799 * Handle Pre-filtering of tables by @anushka-singh in #811 * Email Notification on Share Workflow - Issue - 734 by @TejasRGitHub in #818 * Refactor notifications from core to modules by @dlpzx in #822 * Add frontend and backend feature flags by @zsaltys in #817 * Make hosted_zone_id optional by @lorchda in #812 #### Fixes * Add Additional Error Messages for KMS Key lookup on imported dataset by @noah-paige in #748 * Handle Environment Import of IAM service roles by @noah-paige in #749 * Build Compliant Names for Opensearch Resources by @noah-paige in #750 * Update Lambda runtime by @nikpodsh in #782 * Ensure valid environments for share request and other objects creation by @dlpzx in #781 * Fix shell true semgrep by @dlpzx in #760 * Add condition when there are no public subnets by @lorchda in #794 * Remove unused variable by @zsaltys in #815 * Check other share exists before clean up by @noah-paige in #769 ### Relates - v2.1.0 minor release ## New Contributors * @manjulaK made their first contribution in #786 * @zsaltys made their first contribution in #793 * @anushka-singh made their first contribution in #811 * @TejasRGitHub made their first contribution in #818 ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Noah Paige <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: jaidisido <[email protected]> Co-authored-by: mourya-33 <[email protected]> Co-authored-by: nikpodsh <[email protected]> Co-authored-by: MK <[email protected]> Co-authored-by: Zilvinas Saltys <[email protected]> Co-authored-by: Daniel Lorch <[email protected]> Co-authored-by: Anushka Singh <[email protected]> Co-authored-by: trajopadhye <[email protected]>
Feature or Bugfix
Detail
Relates
N/A
Security
Not relevant
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.