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

Revert Application Error Report from GA #34489

Closed
wants to merge 2 commits into from

Conversation

zandre-eng
Copy link
Contributor

@zandre-eng zandre-eng commented Apr 23, 2024

Product Description

The Application Error Report will no longer be accessible to users, unless the new feature flag is enabled for them.

Technical Summary

Link to ticket here.

The Application Error Report is being moved back into a feature flag. This is because the report has no useful function on production (due to mobile logs being sent to Sumologic).

Feature Flag

A new APPLICATION_ERROR_REPORT feature flag is created.

Safety Assurance

Safety story

  • Local testing of migration (as well as migration revert)
  • Tested on staging

Automated test coverage

N/A

QA Plan

No QA planned.

Migrations

  • The migrations in this code can be safely applied first independently of the code

This PR includes a migration to delete the Role associated with the APPLICATION_ERROR_REPORT privilege. Upon reverting this migration, the Role object will be re-created.

This PR reverts what was done in the migration from this change which moved the APPLICATION_ERROR_REPORT feature flag to a privilege.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@zandre-eng zandre-eng added the product/all-users-all-environments Change impacts all users on all environments label Apr 23, 2024
@dimagimon dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Apr 23, 2024
@zandre-eng zandre-eng marked this pull request as ready for review April 23, 2024 14:38
@mkangia
Copy link
Contributor

mkangia commented Apr 29, 2024

This PR includes a migration to delete the Role associated with the APPLICATION_ERROR_REPORT privilege

Should not this be done as a separate follow up PR to avoid errors on HQ in the interim when migrations are run and the code is updated? So this PR should just remove the use of that privilege in the code first and then ones that has been deployed, we should add a new PR to migrate and remove the redundant privilege?

@zandre-eng
Copy link
Contributor Author

Should not this be done as a separate follow up PR to avoid errors on HQ in the interim when migrations are run and the code is updated? So this PR should just remove the use of that privilege in the code first and then ones that has been deployed, we should add a new PR to migrate and remove the redundant privilege?

@mkangia I was following the process that was done in this PR where the migration and code were done in a single PR. I do agree however, that it might make things easier to have these split off into two PRs. I'll create the necessary PRs and close this one off.

@zandre-eng
Copy link
Contributor Author

@zandre-eng zandre-eng closed this Apr 29, 2024
@zandre-eng zandre-eng deleted the ze/revert-application-error-report-ga branch April 29, 2024 14:12
@mkangia
Copy link
Contributor

mkangia commented Apr 29, 2024

I was following the process that was done in this PR where the migration and code were done in a single PR

@zandre-eng I assume you are referring to where the permission was added. That seems like a safe migration to do before the code changes take place. Removing a record/column are operations that would leave DB in a state that would conflict with the old version of code running on the environment.

@zandre-eng
Copy link
Contributor Author

I assume you are referring to where the permission was added. That seems like a safe migration to do before the code changes take place. Removing a record/column are operations that would leave DB in a state that would conflict with the old version of code running on the environment.

@mkangia Ah apologies, I forgot to add a link to the PR I was referring to in my previous comment. I was following the structure of this PR which I used as an example for how to do a feature revert.

@mkangia
Copy link
Contributor

mkangia commented Apr 30, 2024

Cool, thanks for sharing that @zandre-eng.
I believe we are doing a good approach here with keeping migration separate. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/all-users-all-environments Change impacts all users on all environments reindex/migration Reindex or migration will be required during or before deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants