Skip to content
This repository was archived by the owner on Oct 22, 2024. It is now read-only.

chore: remove desktopapp code fencing#384

Merged
OGPoyraz merged 11 commits intomainfrom
chore/326-remove-all-desktop-app-code-fencing-2
Jan 12, 2023
Merged

chore: remove desktopapp code fencing#384
OGPoyraz merged 11 commits intomainfrom
chore/326-remove-all-desktop-app-code-fencing-2

Conversation

@OGPoyraz
Copy link
Copy Markdown
Member

@OGPoyraz OGPoyraz commented Jan 10, 2023

Overview

This PR aims to remove desktopapp code fencing and replace the functionality with the overriding method.
It also contains the renaming of desktopapp to desktop for code fencing.

@OGPoyraz OGPoyraz changed the title chore: remove desktopapp from Sentry initialisation chore: remove desktopapp code fencing Jan 10, 2023
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We still have to build the desktop app with a build type, but that can be desktopextension and we can achieve the app specific differences using the overrides object and pre-initialisation as you've started to already.

We could rename the build type to desktop to make it less confusing why the app claims to be the desktop extension.

Copy link
Copy Markdown
Member Author

@OGPoyraz OGPoyraz Jan 10, 2023

Choose a reason for hiding this comment

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

Thanks for the clarification, it all make sense now. I will rename it now

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❤️

@OGPoyraz OGPoyraz force-pushed the chore/326-remove-all-desktop-app-code-fencing-2 branch 8 times, most recently from 63bb060 to b1e3558 Compare January 11, 2023 08:04
@OGPoyraz OGPoyraz marked this pull request as ready for review January 11, 2023 08:05
@OGPoyraz OGPoyraz requested a review from a team January 11, 2023 08:05
@OGPoyraz OGPoyraz force-pushed the chore/326-remove-all-desktop-app-code-fencing-2 branch 2 times, most recently from a028811 to dd5ad78 Compare January 11, 2023 10:24
@OGPoyraz OGPoyraz marked this pull request as draft January 11, 2023 10:47
@OGPoyraz OGPoyraz force-pushed the chore/326-remove-all-desktop-app-code-fencing-2 branch 4 times, most recently from e062d38 to 1692193 Compare January 11, 2023 11:42
Comment on lines 7612 to 7642
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was introduced due to the init browser changes on the submodule... I believe that we can set all of those to false (as the app should not fall into the path that will use this dependency).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(to set it to false, you need to add this to the override file and set all those values to false, and then re-run the lavamoat policies generation)

Copy link
Copy Markdown
Member Author

@OGPoyraz OGPoyraz Jan 11, 2023

Choose a reason for hiding this comment

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

Thanks @cryptotavares, now I know how to override policies 🙏
Fixed

Comment on lines 18 to 20
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice one 😎

@OGPoyraz OGPoyraz force-pushed the chore/326-remove-all-desktop-app-code-fencing-2 branch 3 times, most recently from ea599ca to a8569a6 Compare January 11, 2023 12:30
@OGPoyraz OGPoyraz force-pushed the chore/326-remove-all-desktop-app-code-fencing-2 branch from a8569a6 to 27dd896 Compare January 11, 2023 12:32
@OGPoyraz OGPoyraz marked this pull request as ready for review January 11, 2023 12:51
Copy link
Copy Markdown
Contributor

@cryptotavares cryptotavares left a comment

Choose a reason for hiding this comment

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

Looks good

@OGPoyraz OGPoyraz merged commit 2a59761 into main Jan 12, 2023
@OGPoyraz OGPoyraz deleted the chore/326-remove-all-desktop-app-code-fencing-2 branch February 1, 2023 07:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants