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

Prevent changing deps outside nix shell #17662

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

siddarthkay
Copy link
Contributor

@siddarthkay siddarthkay commented Oct 17, 2023

We've often seen cases of devs attempting to change dependencies outside a nix-shell and run into weird side effects
I've been guilt of doing this in the past myself.

This PR stops them from :

  • updating pods outside a nix shell
  • updating node deps outside a nix shell

This PR also cleanup unused scripts in package.json

status: ready

@siddarthkay siddarthkay self-assigned this Oct 17, 2023
@siddarthkay siddarthkay requested a review from jakubgs as a code owner October 17, 2023 12:35
@status-im-auto
Copy link
Member

status-im-auto commented Oct 17, 2023

Jenkins Builds

Click to see older builds (4)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ e632e28 #1 2023-10-17 12:42:24 ~6 min android-e2e 🤖apk 📲
✔️ e632e28 #1 2023-10-17 12:42:41 ~6 min android 🤖apk 📲
✔️ e632e28 #1 2023-10-17 12:43:45 ~7 min ios 📱ipa 📲
✔️ e632e28 #1 2023-10-17 12:45:48 ~9 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ a455889 #2 2023-10-17 13:10:50 ~7 min android-e2e 🤖apk 📲
✔️ a455889 #2 2023-10-17 13:10:52 ~7 min android 🤖apk 📲
✔️ a455889 #2 2023-10-17 13:13:46 ~10 min tests 📄log
✔️ a455889 #2 2023-10-17 13:15:15 ~11 min ios 📱ipa 📲
✔️ b91e628 #5 2023-10-17 18:00:45 ~6 min android-e2e 🤖apk 📲
✔️ b91e628 #5 2023-10-17 18:01:26 ~6 min android 🤖apk 📲
✔️ b91e628 #5 2023-10-17 18:02:41 ~7 min ios 📱ipa 📲
✔️ b91e628 #5 2023-10-17 18:05:14 ~10 min tests 📄log

package.json Outdated Show resolved Hide resolved
Copy link
Member

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

I like how you did it in ios/Podfile, very neat.

As for package.json, honesly we shouldn't be using it at all. There should be only plumbing stuff in there that is required for technical reasons. And a warning to read doc/starting-guide.md.

ios/Podfile Outdated Show resolved Hide resolved
scripts/check-nix-shell.sh Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@siddarthkay siddarthkay force-pushed the prevent-changing-deps-outside-nix-shell branch 2 times, most recently from f370132 to 7513f2d Compare October 17, 2023 17:53
We've often seen cases of devs attempting to change  dependencies outside a nix-shell and run into weird side effects

This commit stops them from :

- updating pods outside a nix shell
- updating node deps outside a nix shell

This commit also cleanup unused scripts in package.json and adds a fake comment script.
@siddarthkay siddarthkay force-pushed the prevent-changing-deps-outside-nix-shell branch from 7513f2d to b91e628 Compare October 17, 2023 17:54
Copy link
Member

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

Looks great.

@siddarthkay
Copy link
Contributor Author

siddarthkay commented Oct 17, 2023

@status-im/mobile-qa : we don't need QA for this PR since its a build change and we don't need to run E2E since these changes won't affect android builds.

@siddarthkay siddarthkay merged commit 6924d99 into develop Oct 17, 2023
@siddarthkay siddarthkay deleted the prevent-changing-deps-outside-nix-shell branch October 17, 2023 18:05
@jakubgs
Copy link
Member

jakubgs commented Oct 17, 2023

This reference was missed:


Fixed: 495aee58

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

Successfully merging this pull request may close these issues.

5 participants