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

AAP-20249: Admin Dashboard: [Feature flag] -M-A-G-I-C- Organizations #804

Closed
wants to merge 1 commit into from

Conversation

manstis
Copy link
Contributor

@manstis manstis commented Feb 2, 2024

Jira Issue: https://issues.redhat.com/browse/AAP-20249

Description

This introduces a magic flag in LaunchDarkly for a (comma-separated) white-list of org_id's that circumvent all known checks for "Schema 2 Telemetry". i.e. if an org_id is present in LaunchDarkly then the "Schema 2 Telemetry" opt-in/out flag will be present in /me irrespective of other settings. Furthermore "Schema 2 Telemetry" events will be sent to Segment.

See #776 for details of local LaunchDarkly test support 🎉

Testing

  1. settings.development.TELEMETRY_SCHEMA_2_ENABLED = True

Using the LaunchDarkly override settings detailed in #776 try with local settings for:

  • "schema_2_telemetry_org_id_white_list": "1234567[*]" - Expect org_telemetry_opt_out: false to be present.
  • "schema_2_telemetry_org_id_white_list": "" - Expect org_telemetry_opt_out to be missing.

For each, execute the api/v0/me endpoint and compare results with the expectations above.

  1. settings.development.TELEMETRY_SCHEMA_2_ENABLED = False

Using the LaunchDarkly override settings detailed in #776 try with local settings for:

  • "schema_2_telemetry_org_id_white_list": "1234567[*]" - Expect org_telemetry_opt_out: false to be present.
  • "schema_2_telemetry_org_id_white_list": "" - Expect org_telemetry_opt_out to be missing.

For each, execute the api/v0/me endpoint and compare results with the expectations above.

[*] 1234567 is whatever org_id your local login belongs to.

Steps to test

  1. Pull down the PR
  2. make start-backends
  3. make create-application
  4. make run-server (or whatever you normally do)

Scenarios tested

See "Testing" above.

Production deployment

  • [] This code change is ready for production on its own
  • This code change requires the following considerations before going to production:

I renamed the system-wide environment variable to TELEMETRY_SCHEMA_2_ENABLED.

This needs to be reflected in Staging Secret Manager.

Copy link
Contributor

@romartin romartin left a comment

Choose a reason for hiding this comment

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

Hey @manstis

Just did a quick look and looks good, but just a small comment: could it be possible refactoring the name of this new flag from schema2 to analytics, WDYT? It is not a blocker at all anyway.

I mention because the schema2 is named everywhere in the code as analytics_telememtry. It's just for being consistent....

@manstis manstis marked this pull request as ready for review February 2, 2024 16:27
@manstis manstis changed the title [DRAFT] AAP-20249: Admin Dashboard: [Feature flag] -M-A-G-I-C- Organizations AAP-20249: Admin Dashboard: [Feature flag] -M-A-G-I-C- Organizations Feb 2, 2024
@manstis
Copy link
Contributor Author

manstis commented Feb 2, 2024

Hi @romartin

...could it be possible refactoring the name of this new flag from schema2 to analytics

To be honest, I renamed the flag in this PR from what it was in main as I thought it wasn't very clear.

The settings.TELEMETRY_SCHEMA_2_ENABLED flag is to be removed when this goes "live" so is short-lived.

Personally, I still prefer to include the version Schema name in the flag.. as we'll no doubt be getting other Schemas in the future.

ansible_wisdom/ai/feature_flags.py Show resolved Hide resolved
Copy link
Contributor

@jameswnl jameswnl left a comment

Choose a reason for hiding this comment

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

the flag usage looks good to me. but I don't fully understand the overriding precedence and so I'll let Roger or others to review that.

@manstis
Copy link
Contributor Author

manstis commented Feb 7, 2024

Closing. The PR description and review discussion will distort review of latest changes.

I'll re-open as a new PR.

@manstis
Copy link
Contributor Author

manstis commented Feb 7, 2024

Replaced by #814

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants