-
Notifications
You must be signed in to change notification settings - Fork 250
Remove swift wrappers from a-s [ci full] #6925
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
Conversation
One challenge here is going to be the story for nimbus tests. Eg, see this test which is referencing a file which is being removed from this repo. What's very odd is that this actually broke when the file was originally moved from the nimbus directory to the ios megazord directory many months ago - and the test do all fail for me locally, but seem to work in CI. We probably need to work with @freshstrangemusic and/or the rest of the nimbus team to work out what's going on here and how important those tests are, because I think they might be salvagable if the files are just moved, but will not be if they are removed. |
5386847
to
4ae78b7
Compare
Per offline discussions, it was discovered these tests never actually ran for quite awhile now and PR merged #6923 to officially "turn off" those tests. |
FTR, the nimbus issues I discussed above have largely been neutered by #6923 edit: haha, sammy and I wrote the same thing at the same time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a rubber stamp, but assuming ios is good with this it lgtm
I can't add @issammani to reviewers so I'll just ping him via this comment. Feel free to ping anyone else that might want a look here. I added the whole a-s team here as after this PR merges, we will officially be changing the pattern of how we dev, cut and release for iOS. We don't need everyone's r+ here just awareness that this process begins via this PR. |
Hey @skhamis. I ran the automation locally and the python fails because we still have a leftover call to application-services/taskcluster/scripts/build-and-test-swift.py Lines 18 to 24 in 4ae78b7
Otherwise this looks good. I also opened mozilla-mobile/firefox-ios#29190 to merge after this to only update files under |
4ae78b7
to
cbd4553
Compare
Great catches! Little worrying that the CI didn't pick up on that (Since CI has been green) but we'll find out quick if things go wrong lol. I'm going to wait to land this until mozilla-mobile/firefox-ios#29190 is also r+'d so we can land these quickly after each other (I don't think we'll miss any but just incase). |
Kicking off the next phase of moving swift infra fully to firefox-ios. Now that https://github.com/mozilla-mobile/firefox-ios/actions/workflows/update-appservices-nightly.yml has been driving the updates no problem and firefox-ios reporting no issues of using the local SPM. We can remove any non-generated swift files as they now live fully in the firefox-ios repo.
Unfortunately due to how we need to generate UniFFi files, we still need to generate then upload those artifacts for them to pull down. However we net some benefits:
main
you can kick off on the firefox-ios side to pull the latest and immediately use.small but noticeable improvements!
Pull Request checklist
[ci full]
to the PR title.