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

fix: flatpak comparison in ujust script #172

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

GarciaLnk
Copy link
Contributor

The previous comparison approach didn't work as newlines in the list were trimmed to form one string with all the flatpaks, and comm compares line by line.

This uses bash arrays to properly manipulate the lists, additionally it sends a notification when new flatpaks are installed.

The previous comparison approach didn't work as newlines in the list were trimmed to form one string with all the flatpaks, and comm compares line by line.

This uses bash arrays to properly manipulate the lists, additionally it sends a notification when new flatpaks are installed.
@GarciaLnk GarciaLnk requested a review from inffy as a code owner February 2, 2025 21:01
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Feb 2, 2025
CURR_LIST_FILE={{ CURR_LIST_FILE }}
IMAGE_NAME=$(jq -r '."image-name"' < "/usr/share/ublue-os/image-info.json")
FLATPAK_LIST="$(curl https://raw.githubusercontent.com/ublue-os/aurora/main/aurora_flatpaks/flatpaks | tr '\n' ' ')"
flatpak --system -y install --or-update ${FLATPAK_LIST}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no clue how this got through lol

CURR_LIST_FILE={{ CURR_LIST_FILE }}
IMAGE_NAME=$(jq -r '."image-name"' < "/usr/share/ublue-os/image-info.json")
FLATPAK_LIST="$(curl https://raw.githubusercontent.com/ublue-os/aurora/main/aurora_flatpaks/flatpaks | tr '\n' ' ')"
flatpak --system -y install --or-update ${FLATPAK_LIST}
FLATPAK_LIST=($(curl https://raw.githubusercontent.com/ublue-os/aurora/main/aurora_flatpaks/flatpaks))
Copy link

@etvt etvt Feb 2, 2025

Choose a reason for hiding this comment

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

Would it be possible / would it make sense to put these files in the image instead of downloading them every time from the internet, in order to:

  1. avoid depending on internet connection to githubusercontent.com on every boot
  2. only consider the historical version of the recommended flatpak list that was active at the point in time when the image was built.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yeah we could curl the list in build time into /usr/share/ublue-os and compare against that in the recipe. I do like that approach much more, on it!

This way we don't need to curl against gh on boot, thx @etvt
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Feb 2, 2025
derp...  I also dont wanna add stuff to the main justfile
Copy link
Collaborator

@ledif ledif 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 the fixes. LGTM!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 3, 2025
@ledif ledif enabled auto-merge February 3, 2025 04:23
@ledif
Copy link
Collaborator

ledif commented Feb 3, 2025

I'm going to force merge this PR because it fixes an issue with code that has already been merged, so it needs to be available in the stable image. The current failing NVIDIA actions are unrelated.

@ledif ledif disabled auto-merge February 3, 2025 04:26
@ledif ledif merged commit 69ed7fa into ublue-os:main Feb 3, 2025
9 of 21 checks passed
Comment on lines +238 to +243
IMAGE_NAME=$(jq -r '."image-name"' < "/usr/share/ublue-os/image-info.json")
FLATPAK_LIST=($(curl https://raw.githubusercontent.com/ublue-os/aurora/main/aurora_flatpaks/flatpaks))

if [[ ${IMAGE_NAME} =~ "dx" ]]; then
FLATPAK_LIST+=($(curl https://raw.githubusercontent.com/ublue-os/aurora/main/dx_flatpaks/flatpaks))
fi
Copy link

Choose a reason for hiding this comment

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

Is there any reason why this else branch is not
FLATPAK_LIST=($(cat /usr/share/ublue-os/flatpak_list))
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It keeps the old behavior of the recipe consistent. When the argument that points to a flatpak list is not given, it installs all the flatpaks from the repo (this only happens when the user manually does ujust install-system-flatpaks)

Copy link

Choose a reason for hiding this comment

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

makes sense, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants