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

Android scaffold #26274

Merged
merged 37 commits into from
Feb 13, 2025
Merged

Android scaffold #26274

merged 37 commits into from
Feb 13, 2025

Conversation

getvictor
Copy link
Member

@getvictor getvictor commented Feb 11, 2025

Android scaffold code and refactorings

  • Android packages intended to be decoupled from other Fleet code

Video explaining the PR: https://www.youtube.com/watch?v=cza-35Z9Wxk

Checklist for submitter

  • If database migrations are included, checked table schema to confirm autoupdate
  • For database migrations:
    • Checked schema for all modified table for columns that will auto-update timestamps during migration.
    • Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
    • Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).
  • Added/updated automated tests
  • Manual QA for all new/changed functionality

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 54.38898% with 530 lines in your changes missing coverage. Please review.

Project coverage is 63.78%. Comparing base (10f44cf) to head (f7e2b00).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
server/mdm/android/service/endpoint_utils.go 0.00% 126 Missing ⚠️
...r/mdm/android/mysql/testing_utils/testing_utils.go 58.73% 67 Missing and 18 partials ⚠️
cmd/fleet/serve.go 0.00% 71 Missing ⚠️
...ervice/middleware/endpoint_utils/endpoint_utils.go 72.19% 42 Missing and 15 partials ⚠️
server/mdm/android/service/service.go 0.00% 48 Missing ⚠️
cmd/fleet/prepare.go 0.00% 33 Missing ⚠️
server/mdm/android/service/handler.go 0.00% 28 Missing ⚠️
server/mdm/android/mysql/errors.go 32.14% 19 Missing ⚠️
server/mdm/android/mysql/enterprises.go 66.66% 10 Missing and 4 partials ⚠️
...tables/20250213104005_AddAndroidEnterpriseTable.go 66.66% 4 Missing and 1 partial ⚠️
... and 22 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #26274      +/-   ##
==========================================
- Coverage   63.88%   63.78%   -0.10%     
==========================================
  Files        1632     1650      +18     
  Lines      157962   158672     +710     
  Branches     4074     4074              
==========================================
+ Hits       100918   101216     +298     
- Misses      49146    49526     +380     
- Partials     7898     7930      +32     
Flag Coverage Δ
backend 64.62% <54.38%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@getvictor getvictor marked this pull request as ready for review February 12, 2025 02:38
@getvictor getvictor requested a review from a team as a code owner February 12, 2025 02:38
@georgekarrv
Copy link
Member

This has a lot more code duplication than I was expecting. I don't remember the engineering team agreeing to continue with this pattern from the Eng together call.

I also don't think the 'small' changes you made in various places will make this easy to support the nuances between main and this separate section of code.

Is there a reason this wasn't done in the main server code like normal?

@getvictor
Copy link
Member Author

This has a lot more code duplication than I was expecting. I don't remember the engineering team agreeing to continue with this pattern from the Eng together call.

I also don't think the 'small' changes you made in various places will make this easy to support the nuances between main and this separate section of code.

Is there a reason this wasn't done in the main server code like normal?

@georgekarrv Any specific areas you're concerned about? I know there are a few places that can be refactored further to remove duplication. We can discuss this at standup or separate meeting.

The current approach is not scalable, and this was an opportunity to try and improve the situation. Given that this feature is still in development, we can refactor it back to the old approach if needed. The current issues with our codebase are:

A significant problem that the engineering team has been facing is an increase in bugs and a longer time to fix them. The code for each feature is sprinkled throughout the codebase and tightly coupled to other seemingly unrelated features. This complexity makes it difficult to understand, test, and keep existing features working as new ones are added.

Speaking of new features, the team has been struggling to add them on time. The codebase has become a tangled web of dependencies, and any change in one part of the codebase can have unintended consequences in other parts. Adding a feature requires modifying many parts of the codebase, which requires understanding the entire codebase, which many developers lack. The lack of knowledge and the changes to many parts of the codebase have led to features taking significantly longer to implement than initially estimated.

Maintaining feature branches for over a few days has become impossible. The codebase is so intertwined that any changes may cause merge conflicts. The increased likelihood of merge conflicts has discouraged developers from refactoring and cleaning up the code base. This tendency to leave the code as-is has perpetuated the slide in code quality.

Tests have also become a problem. The test suite has been in a frequent state of disrepair. There is no clear ownership of tests, so engineers have been reluctant to fix them. Some engineers have stopped paying attention to failing CI alerts, figuring that the problems are caused by one of the other two teams.

Tests have also become slower and slower, especially the integration tests that test the API and include the service layer, the datastore layer, and an actual database. These tests do not run in parallel; every additional feature slows down the compile and increases test time. Test files have become bloated with tests for multiple features, making them slow to load in the editor, difficult to navigate, and impossible to diff for PR reviews.

Copy link
Contributor

@gillespi314 gillespi314 left a comment

Choose a reason for hiding this comment

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

Thanks for these changes, I'm really liking how it is coming together!

@getvictor getvictor merged commit 4b007e2 into main Feb 13, 2025
35 checks passed
@getvictor getvictor deleted the victor/26218-android-turn-on branch February 13, 2025 20:32
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