Skip to content

refact fix: Flatpak#705

Merged
ChrisTitusTech merged 7 commits intoChrisTitusTech:mainfrom
jeevithakannan2:flatpak
Nov 1, 2024
Merged

refact fix: Flatpak#705
ChrisTitusTech merged 7 commits intoChrisTitusTech:mainfrom
jeevithakannan2:flatpak

Conversation

@jeevithakannan2
Copy link
Copy Markdown
Contributor

@jeevithakannan2 jeevithakannan2 commented Sep 30, 2024 •
edited
Loading

Chris read before merging !!

Type of Change

  • New feature
  • Bug fix
  • Documentation update
  • Refactoring
  • Hotfix
  • Security patch
  • UI/UX improvement

Description

  • For the scripts in office suits to work fix: Common script paths #717 must be merged.
  • Moved flatpak handling to common-script due to path conflicts when setup-flatpak is called from different tabs.
  • Added apk package manager in case feat: Alpine linux support  #814 gets merged. If not remove apk package manager before merging
  • This PR marks as a first step if flatpak was decided to be used as a secondary package manager in linutil
  • The following error was reproduced by removing the pacman installation section, forcing the script to use flatpak for installation:

Note

This issue was originally identified on another laptop running openSUSE. Since no screenshots were captured in openSUSE, the workaround was applied to reproduce the error.

flatpak-error

Testing

  • Tested on arch and openSUSE, works as expected.

Additional Description

  • This implementation is basic and can be improved in future.
  • There is a lot of room for improvement here which can be implemented later in the future.

The idea mentioned in #705 (comment) was not implemented because of the following error

image

It occurs because setup-flatpak is also sourcing common-script and the root sourcing happens in the communication dir (zoom). It will try to access common-script in the application setup folder instead of trying to source the common-script from the root directory

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no errors/warnings/merge conflicts.

@ghost

This comment was marked as resolved.

Sign in to view
@ghost

This comment was marked as resolved.

Sign in to view
ghost

This comment was marked as resolved.

Sign in to view

ghost
ghost approved these changes Sep 30, 2024
View reviewed changes
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

if any other scripts use flatpak be sure to include these changes

ghost
ghost reviewed Sep 30, 2024
View reviewed changes
Comment thread core/tabs/applications-setup/bottles-setup.sh
ghost
ghost reviewed Sep 30, 2024
View reviewed changes
Comment thread core/tabs/applications-setup/communication-apps/slack-setup.sh Outdated
ghost
ghost reviewed Sep 30, 2024
View reviewed changes
Comment thread core/tabs/applications-setup/communication-apps/slack-setup.sh Outdated
ghost
ghost reviewed Sep 30, 2024
View reviewed changes
Comment thread core/tabs/applications-setup/communication-apps/zoom-setup.sh Outdated
ghost
ghost reviewed Sep 30, 2024
View reviewed changes
Comment thread core/tabs/applications-setup/communication-apps/zoom-setup.sh Outdated
@ghost
Copy link
Copy Markdown

ghost commented Sep 30, 2024

DONT merge this yet, needs more discussing, this implementation is a bit dirty and i feel like we can just combine the flatpak command_exists with the reg one

@jeevithakannan2 jeevithakannan2 marked this pull request as draft September 30, 2024 14:03
@jeevithakannan2 jeevithakannan2 marked this pull request as ready for review September 30, 2024 16:31
@koibtw koibtw mentioned this pull request Sep 30, 2024
Closed
1 task
@jeevithakannan2 jeevithakannan2 marked this pull request as draft October 9, 2024 14:07
@jeevithakannan2 jeevithakannan2 marked this pull request as ready for review October 9, 2024 14:39
@jeevithakannan2 jeevithakannan2 mentioned this pull request Oct 9, 2024
Closed
3 tasks
@jeevithakannan2
Copy link
Copy Markdown
Contributor Author

jeevithakannan2 commented Oct 9, 2024 •
edited
Loading

DONT merge this yet, needs more discussing, this implementation is a bit dirty and i feel like we can just combine the flatpak command_exists with the reg one

Combined both flatpak command_exists ready to be merged !!

@jeevithakannan2 jeevithakannan2 mentioned this pull request Oct 12, 2024
Merged
12 tasks
koibtw
koibtw approved these changes Oct 30, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@koibtw koibtw left a comment

Choose a reason for hiding this comment

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

I'd wait with this until other PRs that may conflict are resolved.

@ChrisTitusTech ChrisTitusTech merged commit d4eacae into ChrisTitusTech:main Nov 1, 2024
@jeevithakannan2 jeevithakannan2 deleted the flatpak branch November 1, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChrisTitusTech ChrisTitusTech ChrisTitusTech approved these changes

@koibtw koibtw koibtw approved these changes

+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jeevithakannan2 @ChrisTitusTech @koibtw