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

[FIX] fix Slack e2e announcement #5753

Merged
merged 7 commits into from
Feb 16, 2023
Merged

Conversation

NicolasMassart
Copy link
Contributor

@NicolasMassart NicolasMassart commented Feb 10, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions,
1. What is the reason for the change?
Hard to test before running in prod, the start_e2e_tests workflow announcement step fails because of workflow not cloning the repo. However, adding the setup step to have dependencies just for notification is too heavy. Makes the workflow run in 7min when it's only a notification... It can be done in a more simple way.
2. What is the improvement/solution?
Completely replace the JS step by the Bitrise Go Slack step that doesn't require setup, runs faster and does the same thing (even better).

Testing

This has been tested on Bitrise directly with running the workflow using customised env vars to send message to a private slack.

Test Slack resulting message:

image

image

Next step after this PR

  • When we notice that this is fine, we can make this change also for other notifications (build).
  • From this very simple use case, not impacting app code, we can see that there's improvement we can do on the CI. We will start thinking about how to improve and create an improvement ticket.

Q&A to explain why I'm doing it this way.

  • Q: why not adding script in package.json?
    • A: Scripts are a good way to expose app related tasks and things that devs can run locally. Here the notification is none of this, so having it only in CI is better. That's a good thing because it's easier then to just use Bitrise Slack step.
  • Q: Will it make the CI config file harder to migrate if we want to change from Bitrise to something else?
    • A: CI system migration is never easy. Even if you make your best to have abstracted setup.
      However there's a side effect, if you try to abstract things to make sure that they will work on any CI, it means that you will have to stick to common features and have some overlay that will slow down things and make them more complex.
      Also when time will come to move, you will have surprises as CI tools will evolve and what you thought was portable will probably not be.
      So you will not benefit from the power of the current CI tool you are using and will provide a poor dev experience as modifications of the workflows will be more complex. In the end, all this annoying abstraction will make you think that the CI system is not good and you will want to change to another one when the problem is not the CI system, but the way you used it. Then you will reproduce the exact same mistake on the next CI system. So, my advice, go CI native, use CI system 100% and benefit from all the features even if they are the only one to offer that, and especially if they are. Maybe one day you will have to change, we can't tell. But as WAGNI says, we will deal with moving to another CI when we will need to. Until then use the full potential of the one you have.

Issue

fixes MetaMask/mobile-planning#643

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@NicolasMassart NicolasMassart requested a review from a team as a code owner February 10, 2023 11:13
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@NicolasMassart NicolasMassart marked this pull request as draft February 10, 2023 11:14
@NicolasMassart NicolasMassart self-assigned this Feb 10, 2023
@NicolasMassart NicolasMassart changed the title [FIX] fix Slack e2e announcement [FIX] [skip ci] fix Slack e2e announcement Feb 10, 2023
@sethkfman sethkfman added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Feb 14, 2023
@NicolasMassart NicolasMassart force-pushed the fix/643-fix-announcement-step branch 5 times, most recently from ab4f10d to 5df50dc Compare February 15, 2023 12:22
Restore wrongly committed project.pbxproj
@NicolasMassart NicolasMassart marked this pull request as ready for review February 15, 2023 13:22
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@sethkfman
Copy link
Contributor

Can you add the correct labels and merge? Detox Only/No QA Needed and release-6.1

@NicolasMassart NicolasMassart changed the title [FIX] [skip ci] fix Slack e2e announcement [FIX] fix Slack e2e announcement Feb 15, 2023
@NicolasMassart NicolasMassart merged commit f7e44ad into main Feb 16, 2023
@NicolasMassart NicolasMassart deleted the fix/643-fix-announcement-step branch February 16, 2023 11:14
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants