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

[DEVOPS-1733] - Add Feature Branch DB Migration Awareness #248

Merged
merged 8 commits into from
Feb 14, 2024

Conversation

Eeebru
Copy link
Contributor

@Eeebru Eeebru commented Feb 8, 2024

🎟️ Tracking

🚧 Type of change

  • 🤖 Build/deploy pipeline (DevOps)

📔 Objective

📋 Code changes

  • report-deployment-status-to-slack/action.yml: Update Slack deployment report to include caution when Database migration is detected.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@Eeebru Eeebru requested a review from a team as a code owner February 8, 2024 19:44
@bitwarden-bot
Copy link

bitwarden-bot commented Feb 8, 2024

Logo
Checkmarx One – Scan Summary & Details168334ff-41b4-49bc-8fdc-24db261cb0bd

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Unpinned Actions Full Length Commit SHA /test-report-deployment-status-to-slack.yml: 68 Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
MEDIUM Unpinned Actions Full Length Commit SHA /test-report-deployment-status-to-slack.yml: 107 Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
MEDIUM Unpinned Actions Full Length Commit SHA /test-report-deployment-status-to-slack.yml: 94 Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
MEDIUM Unpinned Actions Full Length Commit SHA /test-report-deployment-status-to-slack.yml: 81 Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...

@joseph-flinn joseph-flinn requested a review from a team February 9, 2024 15:21
@joseph-flinn
Copy link
Contributor

Since this action is used heavily, I would like us to come up with a non-invasive way to test these changes before merging in. Testing the changes should not send a message to the #team-eng-qa-devops channel, but instead use #devops-alerts-test as needed.

@joseph-flinn joseph-flinn removed the hold label Feb 12, 2024
@Eeebru
Copy link
Contributor Author

Eeebru commented Feb 12, 2024

Since this action is used heavily, I would like us to come up with a non-invasive way to test these changes before merging in. Testing the changes should not send a message to the #team-eng-qa-devops channel, but instead use #devops-alerts-test as needed.

We have a workflow that runs and tests it when a PR is coming from this composite action file; I'm wondering why it's not running. I'm troubleshooting it.

Copy link
Contributor

@joseph-flinn joseph-flinn left a comment

Choose a reason for hiding this comment

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

We have a workflow that runs and tests it when a PR is coming from this composite action file;

Awesome! Can you add both success and failure test cases for the new db_migration_detected input?

What happens if tag is set to main|rc|hotfix-rc?

@Eeebru
Copy link
Contributor Author

Eeebru commented Feb 12, 2024

We have a workflow that runs and tests it when a PR is coming from this composite action file;

Awesome! Can you add both success and failure test cases for the new db_migration_detected input?

What happens if tag is set to main|rc|hotfix-rc?

I have only tested this action. To test the QA-deploy workflow, this needs to be in main, then I can now test the detect-db-migration job from QA-deploy workflow.

If the tag is set to any of main|rc|hotfix-rc, the detect-db-migration job will be skipped, and the db_migration_detected parameter of the Slack action will be empty. The parameter has a default value of false in this current workflow, so when the detect-db-migration job is skipped, it will still have a value of false.

@joseph-flinn
Copy link
Contributor

joseph-flinn commented Feb 13, 2024

I would like this PR to add success and failure test cases (essentially pipeline unit tests) to the .github/workflows/test-report-deployment-status-to-slack.yml. The test cases should cover the following combinations:

db_migrations_detected tag
true main
true test-branch
false main
false test-branch

Adding these test cases allows us to test the report-deployment-status-to-slack action independent of an end-to-end test (as an example, testing this with the QA-deploy workflow). Maximizing the amount of testing and automated testing prior to merging to main is important to prevent "testing in Production" and the instability risks that it comes with.

@Eeebru
Copy link
Contributor Author

Eeebru commented Feb 13, 2024

Hey @joseph-flinn, could you check this now?

@joseph-flinn joseph-flinn self-requested a review February 13, 2024 22:47
@joseph-flinn
Copy link
Contributor

Thanks for adding the tests, @Eeebru!

@Eeebru Eeebru merged commit 5b6221e into main Feb 14, 2024
6 checks passed
@Eeebru Eeebru deleted the DEVOPS-1733-feature-branch-db-migration-awareness-to-qa branch February 14, 2024 09:59
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.

3 participants