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.

Sign up for GitHub

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

Jump to bottom

Remove postinstall script #942

Merged
merged 25 commits into from
Jan 25, 2022
Merged

Remove postinstall script #942

merged 25 commits into from
Jan 25, 2022

Conversation

Saadnajmi
Copy link
Collaborator

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

Way back in #244 , we added a postinstall script to set the executable bit on our scripts. This was needed because we our publish pipeline at the time was running on a windows VM Image, and the windows filesystem doesn't have an executable bit. We no longer need to run on windows VM images, so this is no longer necessary.

Additionally, post install scripts are a security concern, so it's best we remove unnecessary ones.

This change also removes the last usage of a windows VM in our publish pipeline: the react-native-macos-init publish job.

Changelog

[Internal] [Security] - Removed postinstall script

Test Plan

I can't actually test this as this is a publish pipeline, but I have a few reasons to believe it's safe:

Saadnajmi and others added 22 commits March 22, 2021 14:59
@Saadnajmi
add pull yml
c2b2e2f
@Saadnajmi
Merge pull request #1 from microsoft/master
ab88c7d 
[pull] master from microsoft:master
@pull @ryanlntn
match handleOpenURLNotification event payload with iOS (microsoft#755) (
7a9006b 
#2)

Co-authored-by: Ryan Linton <ryanlntn@gmail.com>
@Saadnajmi
Merge pull request #3 from microsoft/master
497aa72 
[pull] master from microsoft:master
@Saadnajmi
Merge pull request #4 from microsoft/master
1c81e5b 
[pull] master from microsoft:master
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
28aed35
@Saadnajmi
Merge branch 'master' of github.com:Saadnajmi/react-native-macos
62dc473
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
93c7296
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
780b2b7
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
99d5182
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
09e872d
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
9a25530
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
d82a01b
@pull @ntre
@mganandraj @Saadnajmi @ryanlntn
[pull] master from microsoft:master (#11)
d9faa3a 
* Deprecated api (microsoft#853)

* Remove deprecated/unused context param
* Update a few Mac deprecated APIs

* Packing RN dependencies, hermes and ignoring javadoc failure,  (microsoft#852)

* Ignore javadoc failure

* Bringing few more changes from 0.63-stable

* Fixing a patch in engine selection

* Fixing a patch in nuget spec

* Fixing the output directory of nuget pack

* Packaging dependencies in the nuget

* Fix onMouseEnter/onMouseLeave callbacks not firing on Pressable (microsoft#855)

* add pull yml

* match handleOpenURLNotification event payload with iOS (microsoft#755) (#2)

Co-authored-by: Ryan Linton <ryanlntn@gmail.com>

* fix mouse evetns on pressable

* delete extra yml from this branch

* Add macOS tags

* reorder props to have onMouseEnter/onMouseLeave always be before onPress

Co-authored-by: pull[bot] <39814207+pull[bot]@users.noreply.github.com>
Co-authored-by: Ryan Linton <ryanlntn@gmail.com>

* Grammar fixes. (microsoft#856)

Updates simple grammar issues.

Co-authored-by: Nick Trescases <42704557+ntre@users.noreply.github.com>
Co-authored-by: Anandraj <anandrag@microsoft.com>
Co-authored-by: Saad Najmi <saadnajmi2@gmail.com>
Co-authored-by: pull[bot] <39814207+pull[bot]@users.noreply.github.com>
Co-authored-by: Ryan Linton <ryanlntn@gmail.com>
Co-authored-by: Muhammad Hamza Zaman <mh.zaman.4069@gmail.com>
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
39bd488
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
c583ed3
@Saadnajmi
Merge branch 'master' of github.com:Saadnajmi/react-native-macos
9cea270
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
7571092
@Saadnajmi
Merge branch 'master' of github.com:microsoft/react-native-macos
2153ef5
@Saadnajmi
Merge branch 'microsoft:master' into master
47ee4ab
@Saadnajmi
remove postinstall script
168bb04
@Saadnajmi
remove fork specific change
7a25e37
@Saadnajmi Saadnajmi requested a review from alloy as a code owner January 9, 2022 07:16
@pull-bot
Copy link

pull-bot commented Jan 9, 2022 •
edited
Loading

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against 08964d3

@Saadnajmi
Copy link
Collaborator Author

Saadnajmi commented Jan 9, 2022 •
edited
Loading

Fails
🚫
❗ Base Branch - The base branch for this PR is something other than master. Are you sure you want to target something other than the master branch?

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.
Generated by 🚫 dangerJS against 7a25e37

@HeyImChris Probably should fix this somehow too, still think's we're master. EDIT: #946 should do that.

HeyImChris
HeyImChris approved these changes Jan 10, 2022
View reviewed changes
.ado/publish.yml Outdated Show resolved Hide resolved
@Saadnajmi
Copy link
Collaborator Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Saadnajmi Saadnajmi mentioned this pull request Jan 24, 2022
Merged
4 tasks
@Saadnajmi
Copy link
Collaborator Author

Interestingly enough react-native-macos-init hasn't had a new release in 2 years: https://www.npmjs.com/package/react-native-macos-init

This makes sense to me, as I haven't seen any changes to this package in this repo in ~2 years. Regardless, that helps me consider this an even more safe change so I'll go ahead and merge it.

@Saadnajmi Saadnajmi merged commit a51a1d1 into microsoft:main Jan 25, 2022
@Saadnajmi Saadnajmi deleted the remove-postinstall branch January 25, 2022 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kelset kelset kelset left review comments

@HeyImChris HeyImChris HeyImChris approved these changes

@alloy alloy Awaiting requested review from alloy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Saadnajmi @pull-bot @kelset @HeyImChris